Closed Bug 1449954 Opened 6 years ago Closed 6 years ago

UX review for shadow DOM representation in markup view

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: jdescottes, Assigned: ladybenko)

References

Details

Attachments

(2 files)

Bug 1449954 - Slotted node reveal ux.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Combined arrow and link into a single icon
45.56 KB, image/png
Details 
It would be nice to have a UX check of the feature landing in Bug 1053898, even if the UI impact is minimal.
See Also: → 1460613
Product: Firefox → DevTools
Priority: P3 → P2
Victoria: mentioned it with you on slack a while ago, it would be great if you had some cycles to give feedback on the UI/UX of the feature so far.

I listed the main features under https://docs.google.com/document/d/1mfV38BnP_x0l2H7BMddtOys97KqdF9sNPtlF7XPrqg4/edit?usp=sharing if you want to have a look. Otherwise you can play with the feature in Nightly, by using the inspector on a website using webcomponents. Example websites:
- https://juliandescottes.github.io/webcomponents-playground/simple/ 
- https://shop.polymer-project.org/ 

Thanks!
Flags: needinfo?(victoria)
Hi Julian, sorry for the delay on this and thanks for the very easy-to-follow doc!

This looks good. I like that #shadow-root has the same dark styling as DOCTYPE to indicate that it's not a regular DOM elmeent.

The arrows for slotted nodes are nice as well. The one thing I'm wondering is if, instead of having separate arrows and 'reveal' links, we could combine them into one always-showing 'jump to' icon displayed after the element? E.g. version of the Jump-to-debugger icon but upside down, and just one line of "code."

I remember talking to :bwinton about displaying fun curved arrows that point from original to slotted node, but maybe that can come later :)
Flags: needinfo?(victoria)
Thanks for the feedback Victoria! Let's switch to the icon you proposed.

> I remember talking to :bwinton about displaying fun curved arrows that point 
> from original to slotted node, but maybe that can come later :)

Do you mean going through all the nodes between the slotted node and the original one? Would be interested in seeing a mockup if you have any :)
Belén, this looks like a good bug to get started for shadow dom

Some pointers:
- we create the markup for slotted nodes in https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/views/slotted-node-editor.js
- the styles for the current link are at https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/devtools/client/themes/markup.css#334-346

You can probably start testing with the image Victoria mentioned, which we use in the EventTooltip. https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js#133-139

For the real asset to use, maybe Victoria can help - but if you feel comfortable crafting SVGs you can also do it.

We already test the reveal-link, but not the fact that it's displayed only on hover, so I don't think there should be any test impact, or any new test to write.
Flags: needinfo?(balbeza)
(In reply to Julian Descottes (PTO - back July 16th) [:jdescottes][:julian] from comment #3)
> > I remember talking to :bwinton about displaying fun curved arrows that point 
> > from original to slotted node, but maybe that can come later :)
> Do you mean going through all the nodes between the slotted node and the
> original one? Would be interested in seeing a mockup if you have any :)

:D I don't have a mockup, but I think I was envisioning something like the Xcode static analyzer:  
https://s.blogcdn.com/www.tuaw.com/media/2009/09/7aaa540d73ea7edeaacf6892d27c5ed9.jpg
Hi, I'm implementing this using Victoria's first suggestion – combining the link and the separate arrow in one just icon. (I'm using a rotated version of the suggested icon, so I don't think I need assets for this.

Maybe we can later create another bug for the other UX (the arrows thing) ?

Thanks!
Flags: needinfo?(balbeza)
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
Submitted a patch, I'm also attaching a screenshot of how it looks like in the patch
Thanks Belén! The screenshot looks great. It's a bit more subtle the the previous solution - hopefully it's still prominent enough.

(The curved arrows idea probably isn't ready for its own bug :))
Francesco, we're re-using a localization string for a slight different purpose. Before it was a text link ("reveal"), now it's used as the ALT attribute of an icon. You might want to take a look. Thanks!
Flags: needinfo?(francesco.lodolo)
Attachment #8990964 - Flags: review?(bgrinstead)
Comment on attachment 8990964 [details]
Bug 1449954 - Slotted node reveal ux.

https://reviewboard.mozilla.org/r/255950/#review263436

::: commit-message-a675c:1
(Diff revision 1)
> +Bug 1449954 - slotted node reveal ux. r=bgrinstead

Please update the commit message to explain the change (something about switching from a text link to an icon should be sufficient)

::: devtools/client/inspector/markup/views/slotted-node-editor.js:35
(Diff revision 1)
>      this.elt.appendChild(this.tag);
>  
> -    this.revealLink = doc.createElement("span");
> +    this.revealLink = doc.createElement("img");
>      this.revealLink.classList.add("reveal-link");
> -    this.revealLink.textContent = INSPECTOR_L10N.getStr("markupView.revealLink.label");
> +    this.revealLink.src = "chrome://devtools/skin/images/reveal.svg";
> +    this.revealLink.alt = INSPECTOR_L10N.getStr("markupView.revealLink.label");

The way we've done this with other icons in the markup view (i.e. the theme-twisty arrow) is to use a span and:
- set the background image/width/height from CSS
- set the [title] attribute to get tooltip text to show up on hover, something like:  `this.revealLink.setAttribute("title", INSPECTOR_L10N.getStr("markupView.revealLink.label"))`

Just scanning our current button-y / icon-y things in the markup view I think they could use an audit to make sure we are properly setting roles and other attributes (i.e. the 'whitespace only' text node uses role="button", the event bubble doesn't set a role, and the twisty uses role="presentation").

I don't feel really strongly about how you handle it in this bug (other than adding the [title] attribute), but my suggestion would be to follow the existing pattern and then file a separate bug to audit the nodes we are using for button-y things across the markup view  - we can ask for help from the accessibility team to figure out what's best for each case.

::: devtools/client/themes/images/reveal.svg:1
(Diff revision 1)
> +<svg width="16" height="8" xmlns="http://www.w3.org/2000/svg" stroke="context-stroke" fill="none" stroke-linecap="round">

Please copy the header prefix into the top of this file, like in https://searchfox.org/mozilla-central/source/devtools/client/themes/images/accessibility.svg#1-3
Attachment #8990964 - Flags: review?(bgrinstead)
Thanks for flagging. Please add a new string: the context is different enough that some languages will use a different style (declarative vs imperative), and even in English a tooltip that starts lowercase doesn't look right.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8990964 [details]
Bug 1449954 - Slotted node reveal ux.

https://reviewboard.mozilla.org/r/255950/#review263436

Thanks for the review! I also created a new localization string for the tooltip, as :flod suggested.

> The way we've done this with other icons in the markup view (i.e. the theme-twisty arrow) is to use a span and:
> - set the background image/width/height from CSS
> - set the [title] attribute to get tooltip text to show up on hover, something like:  `this.revealLink.setAttribute("title", INSPECTOR_L10N.getStr("markupView.revealLink.label"))`
> 
> Just scanning our current button-y / icon-y things in the markup view I think they could use an audit to make sure we are properly setting roles and other attributes (i.e. the 'whitespace only' text node uses role="button", the event bubble doesn't set a role, and the twisty uses role="presentation").
> 
> I don't feel really strongly about how you handle it in this bug (other than adding the [title] attribute), but my suggestion would be to follow the existing pattern and then file a separate bug to audit the nodes we are using for button-y things across the markup view  - we can ask for help from the accessibility team to figure out what's best for each case.

Thanks! I switched the IMG for an empty span and setting the icon via CSS plus a title. I'm not sure about how accessibility works in the devtools panels, but if this were a normal website / webapp, I'd strongly suggest using an IMG to represent icons that are not decoration. As you suggested, I'll create a separate bug to review what the best approach is.
Comment on attachment 8990964 [details]
Bug 1449954 - Slotted node reveal ux.

https://reviewboard.mozilla.org/r/255950/#review263720

Thanks!

::: devtools/client/locales/en-US/inspector.properties:91
(Diff revision 2)
> -# LOCALIZATION NOTE (markupView.revealLink.label)
> -# Used in the markup view when displaying elements inserted in <slot> nodes in a custom
> -# component. On hover, a link with this label will be shown to select the corresponding
> -# non-slotted container. (test with dom.webcomponents.shadowdom.enabled set to true)
> -markupView.revealLink.label=reveal
> +# LOCALIZATION NOTE (markupView.revealLink.toolip)
> +# Used as a tooltip for an icon in the markup view when displaying elements inserted in
> +# <slot> nodes in a custom  component. When clicking on the icon, the corresponding
> +# non-slotted container will be selected
> +# (test with dom.webcomponents.shadowdom.enabled set to true)
> +markupView.revealLink.tooltip=Reveal

Note: we would usually need to rev the ID here if the string changes (i.e. markupView.revealLink.tooltip1). But I believe it's OK because this is just a case change.
Attachment #8990964 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8990964 [details]
> Bug 1449954 - Slotted node reveal ux.
> 
> https://reviewboard.mozilla.org/r/255950/#review263720
> 
> Thanks!
> 
> ::: devtools/client/locales/en-US/inspector.properties:91
> (Diff revision 2)
> > -# LOCALIZATION NOTE (markupView.revealLink.label)
> > -# Used in the markup view when displaying elements inserted in <slot> nodes in a custom
> > -# component. On hover, a link with this label will be shown to select the corresponding
> > -# non-slotted container. (test with dom.webcomponents.shadowdom.enabled set to true)
> > -markupView.revealLink.label=reveal
> > +# LOCALIZATION NOTE (markupView.revealLink.toolip)
> > +# Used as a tooltip for an icon in the markup view when displaying elements inserted in
> > +# <slot> nodes in a custom  component. When clicking on the icon, the corresponding
> > +# non-slotted container will be selected
> > +# (test with dom.webcomponents.shadowdom.enabled set to true)
> > +markupView.revealLink.tooltip=Reveal
> 
> Note: we would usually need to rev the ID here if the string changes (i.e.
> markupView.revealLink.tooltip1). But I believe it's OK because this is just
> a case change.

Actually, I just saw Comment 13. In this case we *do* want to rev the ID so that it can be retranslated. Basically: find/replace markupView.revealLink.tooltip with markupView.revealLink.tooltip1.
Flags: needinfo?(balbeza)
Nevermind about the last two comments - I totally missed that you already changed the ID from label->tooltip.
Flags: needinfo?(balbeza)
Comment on attachment 8990964 [details]
Bug 1449954 - Slotted node reveal ux.

https://reviewboard.mozilla.org/r/255950/#review263730

::: devtools/client/locales/en-US/inspector.properties:86
(Diff revision 2)
>  # LOCALIZATION NOTE (markupView.newAttribute.label)
>  # This is used to speak the New Attribute button when editing a tag
>  # and a screen reader user tabs to it. This string is not visible onscreen.
>  markupView.newAttribute.label=New attribute
>  
> -# LOCALIZATION NOTE (markupView.revealLink.label)
> +# LOCALIZATION NOTE (markupView.revealLink.toolip)

nit: typo (toolip)

::: devtools/client/locales/en-US/inspector.properties:91
(Diff revision 2)
> -# LOCALIZATION NOTE (markupView.revealLink.label)
> -# Used in the markup view when displaying elements inserted in <slot> nodes in a custom
> -# component. On hover, a link with this label will be shown to select the corresponding
> -# non-slotted container. (test with dom.webcomponents.shadowdom.enabled set to true)
> -markupView.revealLink.label=reveal
> +# LOCALIZATION NOTE (markupView.revealLink.toolip)
> +# Used as a tooltip for an icon in the markup view when displaying elements inserted in
> +# <slot> nodes in a custom  component. When clicking on the icon, the corresponding
> +# non-slotted container will be selected
> +# (test with dom.webcomponents.shadowdom.enabled set to true)
> +markupView.revealLink.tooltip=Reveal

The string ID was markupView.revealLink.label and now is markupView.revealLink.tooltip, so that's OK.
Thanks, just fixed the typo in the comment Francesco pointed out.
Keywords: checkin-needed
I couldn't land your patch. ladybenko: Please set the issues opened by the reviewer as fixed by commit so review board allows to land them. Thank you.
Flags: needinfo?(balbeza)
Hi, thanks Eliza, I just marked the remaining issues as fixed. Note that although review board is still showing the icon of unresolved issues, in the issue summary https://reviewboard.mozilla.org/r/255950/#issue-summary all of them are fixed (I've been told this is a glitch)
Flags: needinfo?(balbeza)
Blocks: 1460613
https://hg.mozilla.org/mozilla-central/rev/3efbcb68c621
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
I have reproduced this issue using Firefox 61.0a1 (2018.03.29) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 63.0b3 on Ubuntu 16.04 x64, Windows  10 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: