Open Bug 1205371 Opened 4 years ago Updated Last year

Visualize multiple pseudo class locks on an element in the markup view

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: hholmes, Unassigned)

References

Details

(Whiteboard: [devtools-ux])

Attachments

(4 files, 9 obsolete files)

81.62 KB, image/png
Details
835.46 KB, image/jpeg
Details
6.43 KB, patch
ntim
: review+
Details | Diff | Splinter Review
16.54 KB, image/png
Details
Currently there's no way to express multiple pseudo states on an element (we have orange dots for a pseudo state existing, but no way to express an element having both a :hover and an :active state applied).

Quick sketch of a suggested solution attached along with a screenshot of the current way we're showing it. I'll mock up something in higher fidelity as well if people think this is a good solution.
Attached image IMG_1404.jpg
I think that's a great idea.
So, if the different parts of the lock icon have specific colors, maybe we should re-use these colors in the pseudo-class lock menu in the rule-view (the one just below the filter style, when you click on the expand icon), and maybe they should also be repeated in the inspector right-click contextual menu (although I don't know if we can do that).
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> I think that's a great idea.
> So, if the different parts of the lock icon have specific colors, maybe we
> should re-use these colors in the pseudo-class lock menu in the rule-view
> (the one just below the filter style, when you click on the expand icon),
> and maybe they should also be repeated in the inspector right-click
> contextual menu (although I don't know if we can do that).

I like that—it ends up acting as a legend for what the colors mean.
Assignee: nobody → hholmes
See Also: → 1200686
Depends on: 1120406
WIP - this converts the pseudo class circle into a node so it can be styled with multiple locks
Summary: No way to see that multiple pseudo classes are applied to an element → Visualize multiple pseudo class locks on an element in the markup view
Really a WIP, just imported some code from Helen's demo [0] modified to use em units and adapting to change colors based on which locks are applied.

Note that there is something off with the math a bit due to the import so the 2 and 3 lock states aren't a perfect circle.

[0]: http://codepen.io/helenvholmes/pen/JYEzgK
Attachment #8670351 - Attachment is obsolete: true
For future reference, here's a jsfiddle we were using to play around with the CSS: http://jsfiddle.net/84sqasyk/
See Also: → 1183743
Attached patch bug1205371.patch (obsolete) — Splinter Review
Rebased bgrins' code. (He's on PTO though, so I'll take the code reviews I guess.)

There are some issues that will become apparent when testing this patch; I'll attach the bug numbers once they're filed.
Attachment #8670375 - Attachment is obsolete: true
Attachment #8670448 - Attachment is obsolete: true
Attachment #8724246 - Flags: review?(ntim.bugs)
Attached patch bug1205371.patch (obsolete) — Splinter Review
Attachment #8724246 - Attachment is obsolete: true
Attachment #8724246 - Flags: review?(ntim.bugs)
Attachment #8724247 - Flags: review?(ntim.bugs)
Comment on attachment 8724246 [details] [diff] [review]
bug1205371.patch

Review of attachment 8724246 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/markup/markup.js
@@ +2144,3 @@
>      if (this.node.pseudoClassLocks.length) {
> +      for (let state of this.node.pseudoClassLocks) {
> +        if (state === ":hover") {

It would be cleaner to use a switch statement here.

::: devtools/client/themes/markup.css
@@ +243,5 @@
> +   left: 1px;
> +  overflow: hidden;
> +   position: absolute;
> + }
> + 

NIT whitespace
Attachment #8724246 - Attachment is obsolete: false
Depends on: 1251755
See Also: → 1251758
Comment on attachment 8724246 [details] [diff] [review]
bug1205371.patch

Review of attachment 8724246 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to have a test for this. Let me know if you need help with the test.

::: devtools/client/inspector/markup/markup.js
@@ +2144,2 @@
>      if (this.node.pseudoClassLocks.length) {
> +      for (let state of this.node.pseudoClassLocks) {

This formatting would be shorter: 
let {pseudoClassLocks} = this.node;
if (pseudoClassLocks.length) {
  for (let state of pseudoClassLocks) {

@@ +2144,3 @@
>      if (this.node.pseudoClassLocks.length) {
> +      for (let state of this.node.pseudoClassLocks) {
> +        if (state === ":hover") {

I disagree with :gl, a switch statement wouldn't be appropriate in this case, since we want all 3 blocks to be evaluated.

@@ +2152,5 @@
> +        if (state === ":focus") {
> +          this.elt.classList.add("pseudoclass-focus");
> +        }
> +      }
> +      this.elt.setAttribute("pseudoclass-count", this.node.pseudoClassLocks.length);

nit: this line is over 80 chars, you can use the variable I suggested above to reduce the number of chars.

::: devtools/client/themes/markup.css
@@ +223,5 @@
>  
> +.child[pseudoclass-count] {
> +  --hover-pseudo-background: var(--theme-highlight-green);
> +  --active-pseudo-background: var(--theme-highlight-red);
> +  --focus-pseudo-background: var(--theme-highlight-lightorange);

As we talked about on IRC, it would be nice to increase the contrast of the focus background.

Seems like var(--theme-highlight-purple) would make a great candidate.

@@ +226,5 @@
> +  --active-pseudo-background: var(--theme-highlight-red);
> +  --focus-pseudo-background: var(--theme-highlight-lightorange);
> +}
> +
> +.pseudoclass-locked-container {

Can you replace this selector with .child:not([pseudoclass-count]) > .pseudoclass-locked-container and remove the `display: block;` property from the next block ?

That way it's much more explicit what this rule does.

@@ +232,5 @@
> +}
> +
> + /* Draw a circle next to nodes that have a pseudo class lock.
> +    Center vertically with the 1.4em line height on .tag-line */
> +.child[pseudoclass-count] > .pseudoclass-locked-container {

nit: funky indentation on the whole block

@@ +243,5 @@
> +   left: 1px;
> +  overflow: hidden;
> +   position: absolute;
> + }
> + 

nit: whitespace (as :gl pointed out)

@@ +254,5 @@
> +.child[pseudoclass-count="1"].pseudoclass-focus > .pseudoclass-locked-container {
> +  background: var(--focus-pseudo-background);
> +}
> +
> +

Please use comments to separate each section, rather than 2 lines
Like this:
/* 1 pseudo class lock */

...

/* 2 pseudo class locks */

...

/* 3 pseudo class locks */

...

@@ +257,5 @@
> +
> +
> +.child[pseudoclass-count="2"] > .pseudoclass-locked-container::before {
> +  content: "";
> +  position:absolute;

nit: space after position:

@@ +259,5 @@
> +.child[pseudoclass-count="2"] > .pseudoclass-locked-container::before {
> +  content: "";
> +  position:absolute;
> +  width: 100%;
> +  height: 100%;

Can we avoid code duplication by sharing as much of these rules as we can? Specifically the content/position/width/height rules.

@@ +264,5 @@
> +  background: var(--active-pseudo-background);
> +}
> +.child[pseudoclass-count="2"] > .pseudoclass-locked-container::after {
> +  content: "";
> +  position:absolute;

nit: space after position:

@@ +288,5 @@
> +  transform: rotate(90deg) skew(30deg);
> +  width: 0.53333em;
> +  margin-left: -0.10429em;
> +  margin-top: 0.05em;
> +  height: 0.47059em;

These em lengths are intriguing, if they follow some mathematical formula, I would prefer them to appear explicitly using calc(), if not, it's fine to put them this way

@@ +289,5 @@
> +  width: 0.53333em;
> +  margin-left: -0.10429em;
> +  margin-top: 0.05em;
> +  height: 0.47059em;
> +  content: '';

nit: double quotes

@@ +298,5 @@
> +  transform: rotate(30deg) skew(30deg);
> +  width: 0.53333em;
> +  height: 0.47059em;
> +  margin-left: 0.35783em;
> +  margin-top: -0.47em;

Same comment about the em lengths

@@ +299,5 @@
> +  width: 0.53333em;
> +  height: 0.47059em;
> +  margin-left: 0.35783em;
> +  margin-top: -0.47em;
> +  content: '';

nit: double quotes
Attachment #8724246 - Flags: feedback+
Comment on attachment 8724247 [details] [diff] [review]
bug1205371.patch

Seems like I got to review the wrong patch, anyway, they seem almost identical, so the comments from Comment 11 apply as well.
Attachment #8724247 - Flags: review?(ntim.bugs) → feedback+
Attachment #8724246 - Attachment is obsolete: true
Attached patch bug1205371.patch (obsolete) — Splinter Review
Rebased and addressed some of my review comments.
Attachment #8724247 - Attachment is obsolete: true
Assignee: hholmes → ntim.bugs
Attachment #8790040 - Flags: review?(gl)
Attachment #8735441 - Attachment is obsolete: true
Attachment #8790042 - Flags: review?(gl)
Attachment #8790040 - Attachment is obsolete: true
Attachment #8790040 - Flags: review?(gl)
Comment on attachment 8790042 [details] [diff] [review]
Multi-state pseudo-class lock indicator

Review of attachment 8790042 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming my comments will be addressed. Needinfo me if you need anything. I would also do uireview with Helen - either providing a trybuild or screenshots (preferably with the different themes). I didn't see a spec for the colors to be used, but I assume we might want to change it a little for the different themes.

::: devtools/client/inspector/markup/markup.js
@@ +2508,5 @@
> +    this.elt.classList.remove("pseudoclass-hover");
> +    this.elt.classList.remove("pseudoclass-active");
> +    this.elt.classList.remove("pseudoclass-focus");
> +    this.elt.removeAttribute("pseudoclass-count");
> +    let {pseudoClassLocks} = this.node;

Could use some new lines to separate out logically blocks. For instance, add a new line between #2511 and #2512 since the first block clears the classes attributes.

@@ +2509,5 @@
> +    this.elt.classList.remove("pseudoclass-active");
> +    this.elt.classList.remove("pseudoclass-focus");
> +    this.elt.removeAttribute("pseudoclass-count");
> +    let {pseudoClassLocks} = this.node;
> +    if (pseudoClassLocks.length) {

Add a new line before starting the if statement block

::: devtools/client/themes/markup.css
@@ +259,5 @@
> +
> +.child[pseudoclass-count] {
> +  --hover-pseudo-background: var(--theme-highlight-green);
> +  --active-pseudo-background: var(--theme-highlight-red);
> +  --focus-pseudo-background: var(--theme-highlight-blue);

I would prefer for us to keep our variable assignments at the top as seen in rules.css. I am assuming we will have different background colors for the different themes.

I haven't seen us put variables inside a more specific element selector because of the theming issue. Not quite sure if we are adapting more of this practice or not, but I prefer seeing variables at the top of the stylesheet so we know what kind of variables are being used in the stylesheet since these local variables are referring a more global variable from variables.css

@@ +273,4 @@
>    border-radius: 50%;
>    width: .8em;
>    height: .8em;
>    margin-top: .3em;

I think the circle can be further improved to be vertically centered. .2em makes it look slightly more centered. I will attach a screenshot to illustrate what i am talking about.

@@ +273,5 @@
>    border-radius: 50%;
>    width: .8em;
>    height: .8em;
>    margin-top: .3em;
> +  border-radius: 50%;

This can be removed since we already defined 'border-radius: 50%'

@@ +280,3 @@
>    position: absolute;
>    z-index: 1;
> +  border: 1px solid #fff;

I would move border above border-radius to group them together.
Attachment #8790042 - Flags: review?(gl) → review+
Will attach screenshots for ui-review
Attachment #8790042 - Attachment is obsolete: true
Attachment #8791742 - Attachment is obsolete: true
Attachment #8794603 - Flags: review+
Attached image pseudo-class-locks.png
I've used blue for focus, because it refers to the blue focus border we see on inputs, buttons on lots of platforms.

Hover and Active are just really randomly chosen though (but with contrast in consideration)
Attachment #8794605 - Flags: ui-review?(hholmes)
Comment on attachment 8794605 [details]
pseudo-class-locks.png

Looks like they have a 1px white border; can we remove that? Because we're adding more information and not removing any, I'm not concerned about it being 100% visible in all situations (such as in the highlighted line).

Otherwise, looking good!
Attachment #8794605 - Flags: ui-review?(hholmes)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/d64bcb6d78e5
Multi-state pseudo-class lock indicator. r=gl
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #20)
> Comment on attachment 8794605 [details]
> pseudo-class-locks.png
> 
> Looks like they have a 1px white border; can we remove that? Because we're
> adding more information and not removing any, I'm not concerned about it
> being 100% visible in all situations (such as in the highlighted line).

Removed!
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #19)
> Created attachment 8794605 [details]
> pseudo-class-locks.png
> ...
> Hover and Active are just really randomly chosen though (but with contrast in consideration)
:hover  - green
:active - red
:focus  - blue

Current indicators don't allow to remember / develop mnemonics for them. Order of pseudoclasses in
ruleview (:hover - :active - :focus) should match the order of colors in RGB system (red-green-blue).
Therefore I suggest you 2 options for making mnemonics easier (X is preferable):

X) Colors for :hover and :active should switch places:
:hover  - red
:active - green
:focus  - blue

Y) :hover and :active checkboxes in ruleview (and menuitems in context menu) should switch places:
1st - :active (red)
2nd - :hover  (green)
3rd - :focus  (blue)
(In reply to arni2033 from comment #24)
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #19)
> > Created attachment 8794605 [details]
> > pseudo-class-locks.png
> > ...
> > Hover and Active are just really randomly chosen though (but with contrast in consideration)
> :hover  - green
> :active - red
> :focus  - blue
> 
> Current indicators don't allow to remember / develop mnemonics for them.
> Order of pseudoclasses in
> ruleview (:hover - :active - :focus) should match the order of colors in RGB
> system (red-green-blue).
> Therefore I suggest you 2 options for making mnemonics easier (X is
> preferable):
> 
> X) Colors for :hover and :active should switch places:
> :hover  - red
> :active - green
> :focus  - blue
I like this idea. It also keeps the fact that blue matches the :focus border used across platforms.

> Y) :hover and :active checkboxes in ruleview (and menuitems in context menu)
> should switch places:
> 1st - :active (red)
> 2nd - :hover  (green)
> 3rd - :focus  (blue)
:hover, :active, :focus is the familiar order used in old Firefox and Chrome, so it's better to leave the order untouched.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01303a5f2be
Multi-state pseudo-class lock indicator. r=gl
Backed out for failing devtools test browser_markup_void_elements_html.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7b31d1d24343dfd0e73df4478a2c0e6afb3d5a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a01303a5f2bef7deeaa3248ead8d353b5c0eb041
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39129165&repo=mozilla-inbound

04:29:09     INFO - TEST-PASS | devtools/client/inspector/markup/test/browser_markup_void_elements_html.js | hr closing tag is hidden - 
04:29:09     INFO - check expanded void element closing tag is not hidden
04:29:09     INFO - Waiting for queued children updates to be handled
04:29:09     INFO - TEST-PASS | devtools/client/inspector/markup/test/browser_markup_void_elements_html.js | hr container is expanded - 
04:29:09     INFO - TEST-INFO | started process screenshot
04:29:09     INFO - TEST-INFO | screenshot: exit 0
04:29:09     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_void_elements_html.js | hr closing tag is not hidden anymore - 
04:29:09     INFO - Stack trace:
04:29:09     INFO -     chrome://mochitests/content/browser/devtools/client/inspector/markup/test/browser_markup_void_elements_html.js:null:43
04:29:09     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
04:29:09     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
04:29:09     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Unassigning to reflect real status.
Assignee: ntim.bugs → nobody
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.