Skip to content

Conversation

@crisbeto
Copy link
Member

Since the color contrast on the expansion panel focus state isn't great, it's a good candidate to have strong focus indication.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Feb 19, 2020
@crisbeto crisbeto requested a review from jelbourn as a code owner February 19, 2020 21:07
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 19, 2020
@jelbourn jelbourn requested a review from zelliott February 20, 2020 17:19
@jelbourn
Copy link
Member

@zelliott PTAL

@zelliott
Copy link
Collaborator

@crisbeto The reason we removed the expansion panel header focus indicators from the master focus indicators PR (8a44fb9) was because adding position: relative to the expansion panel headers was causing box-shadow layering issues in certain scenarios.

The TL;DR is when an expansion panel header has a non-transparent background & position: relative, the non-transparent background is layered on top of the parent mat-expansion-panel box-shadow. The diff is subtle:

No position: relative (note the box-shadow between headers):

Screen Shot 2020-02-20 at 10 26 41 AM

position: relative (note the lack of box-shadow between headers):

Screen Shot 2020-02-20 at 10 26 27 AM

We tried to fix this layering issue by adding position: relative to the parent mat-expansion-panel as well. However, this caused more diffs to the mat-expansion-panel box-shadow.

I think a more involved CSS fix will be needed to make these expansion panel headers relatively positioned...

@crisbeto
Copy link
Member Author

Hmm, I think I see the difference in your screenshots, but I don't see anything if I try it out for myself. Here are a couple of examples. The first one is without relative and the second one is with.

Expansion_Panel__Angular_Material_-_Google_Chrome_2020-02-20_20-58-35
Expansion_Panel__Angular_Material_-_Google_Chrome_2020-02-20_20-58-53

@zelliott
Copy link
Collaborator

@crisbeto Try adding a non-transparent background to the headers? The headers have a transparent background by default, so the layering has no visual effect.

Since the color contrast on the expansion panel focus state isn't great, it's a good candidate to have strong focus indication.
@crisbeto crisbeto force-pushed the expansion-panel-strong-focus branch from 583772a to 879c4d2 Compare February 20, 2020 20:53
Copy link
Collaborator

@zelliott zelliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 24, 2020
@mmalerba mmalerba added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Mar 24, 2020
@mmalerba mmalerba merged commit f405cb6 into angular:master Apr 8, 2020
mmalerba pushed a commit that referenced this pull request Apr 14, 2020
Since the color contrast on the expansion panel focus state isn't great, it's a good candidate to have strong focus indication.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants