Closed Bug 1473901 Opened 6 years ago Closed 6 years ago

Show if a shadow root is open or closed

Categories

(DevTools :: Inspector, enhancement, P3)

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

(1 file)

Bug 1473901 - show shadow root mode.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
After Bug 1449333 we now support both open and closed shadow roots.

The #shadow-root node in the markup view could add a visual indicator to show if the shadow root is open or closed. For instance chrome devtools add "(open)" or "(closed)" after #shadow-root.
Blocks: 1473902
Flags: needinfo?(balbeza)
This is probably another good bug for shadow dom work.

We can start simply with adding (open) or (closed) next to #shadow-root in the markup view.
The markup for #shadow-root elements is created in the read-only-editor.

The read-only-editor is on the client side, and therefore has access to nodeFront objects. A front represents a server side object giving you access to some serialized properties and methods. Look at https://docs.firefox-dev.tools/backend/protocol.js.html for more details. The most important to remember in the scope of this bug is that the object returned by NodeActor's form() method is serialized and accessible by the NodeFront at this._form.

So here you will have to add a new information in the NodeActor form (eg isClosedShadowRoot?), maybe add a small function in the NodeFront to read this from _form, and finally use this information in read-only-editor.js

node actor: https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js
node front: https://searchfox.org/mozilla-central/source/devtools/shared/fronts/node.js
read-only-editor: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/views/read-only-editor.js

For tests, you can have a look at anything in https://searchfox.org/mozilla-central/search?q=&case=true&regexp=false&path=browser_markup_shadow.
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
Flags: needinfo?(balbeza)
Comment on attachment 8991629 [details]
Bug 1473901 - show shadow root mode.

https://reviewboard.mozilla.org/r/256552/#review263410


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/markup/views/read-only-editor.js:15
(Diff revision 1)
> +  new LocalizationHelper("devtools/client/locales/inspector.properties");
> +
> +const SHADOW_MODES_L10N = {
> +  open: INSPECTOR_L10N.getStr("markupView.shadowDOM.mode.open"),
> +  closed: INSPECTOR_L10N.getStr("markupView.shadowDOM.mode.closed"),
> +}

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8991629 [details]
Bug 1473901 - show shadow root mode.

https://reviewboard.mozilla.org/r/256552/#review263560

::: devtools/client/locales/en-US/inspector.properties:42
(Diff revision 2)
>  # Used in a tooltip that appears when the user hovers over whitespace-only text nodes in
>  # the inspector.
>  markupView.whitespaceOnly=Whitespace-only text node: %S
>  
> +# LOCALIZATION NOTE
> +# Used to indicate the mode of a shadow root node (open or closed)

I'm wondering if we want to add a note *not* to localize this string, since 'open' and 'closed' is defined by the API: https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/mode.

In fact, I wonder if we should even add this as a string in the properties file vs directly setting it as textContent in the UI. Going to needinfo l10n for guidance here.
Attachment #8991629 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8991629 [details]
> Bug 1473901 - show shadow root mode.
> 
> https://reviewboard.mozilla.org/r/256552/#review263560
> 
> ::: devtools/client/locales/en-US/inspector.properties:42
> (Diff revision 2)
> >  # Used in a tooltip that appears when the user hovers over whitespace-only text nodes in
> >  # the inspector.
> >  markupView.whitespaceOnly=Whitespace-only text node: %S
> >  
> > +# LOCALIZATION NOTE
> > +# Used to indicate the mode of a shadow root node (open or closed)
> 
> I'm wondering if we want to add a note *not* to localize this string, since
> 'open' and 'closed' is defined by the API:
> https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/mode.
> 
> In fact, I wonder if we should even add this as a string in the properties
> file vs directly setting it as textContent in the UI. Going to needinfo l10n
> for guidance here.

Axel - since :flod is away, would you be able to weigh in here or redirect?
Flags: needinfo?(axel)
Comment on attachment 8991629 [details]
Bug 1473901 - show shadow root mode.

https://reviewboard.mozilla.org/r/256552/#review263560

> I'm wondering if we want to add a note *not* to localize this string, since 'open' and 'closed' is defined by the API: https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/mode.
> 
> In fact, I wonder if we should even add this as a string in the properties file vs directly setting it as textContent in the UI. Going to needinfo l10n for guidance here.

I was wondering the same, since I'm not sure if we are showing the API value or we are showing a "human-friendly interpretation". If we go to show the API value straight away, I also agree to remove the strings from the localization file.
Comment on attachment 8991629 [details]
Bug 1473901 - show shadow root mode.

https://reviewboard.mozilla.org/r/256552/#review263632

I'm torn on wether to localize the shadow-root or not. I've looked at MDN localizations, and there's not a lot yet. But at least some keep 'shadow root' intact, so maybe it's a good idea to do that in the UI, too.

Do you have any general policies on that?

I left some notes on general problems with hard-coded strings in this patch for educational purposes only ;-)

::: devtools/client/inspector/markup/views/read-only-editor.js:35
(Diff revision 2)
>    } else if (node.nodeType == nodeConstants.DOCUMENT_TYPE_NODE) {
>      this.elt.classList.add("comment", "doctype");
>      this.tag.textContent = node.doctypeString;
>    } else if (node.isShadowRoot) {
> -    this.tag.textContent = "#shadow-root";
> +    this.tag.textContent = "#shadow-root" +
> +      ` (${SHADOW_MODES_L10N[node.shadowRootMode]})`;

This is hard-coded UI text: " (" and ")".

This should be part of the localized string.

Best way would probably be to expose both

#shadow-root (closed)
#shadow-root (open)

as two separate strings.

Or not at all ;-).

::: devtools/client/locales/en-US/inspector.properties:42
(Diff revision 2)
>  # Used in a tooltip that appears when the user hovers over whitespace-only text nodes in
>  # the inspector.
>  markupView.whitespaceOnly=Whitespace-only text node: %S
>  
> +# LOCALIZATION NOTE
> +# Used to indicate the mode of a shadow root node (open or closed)

Ride along note, the localization comment here isn't going to make it to the second string, pontoon will only show it on the first.
Attachment #8991629 - Flags: review-
Flags: needinfo?(axel)
Hi, after speaking with Julian, we both agree that `open` and `closed` are API values and shouldn't be localized –as Brian suggested. So I have removed the strings from the localization file and they are now hardcoded instead as the `textContent` of the node.

Thanks!
Axel, are you OK with this not being localized? Thanks :)
Flags: needinfo?(l10n)
(In reply to Belén [:ladybenko] from comment #11)
> Axel, are you OK with this not being localized? Thanks :)

Yes, it's OK to keep it hard-coded in this case.
Flags: needinfo?(l10n)
Fixed eslint errors in the test file
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 01eece0d8ffccd9a22bbcbb58ae65414757606df -d 050d2e0e3b62: rebasing 473464:01eece0d8ffc "Bug 1473901 - show shadow root mode. r=bgrins" (tip)
merging devtools/client/inspector/markup/test/browser.ini
merging devtools/server/actors/inspector/node.js
warning: conflicts while merging devtools/client/inspector/markup/test/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by balbeza@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98f850870874
show shadow root mode. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/98f850870874
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
I have reproduced this issue using Firefox 63.0a1 (2018.07.06) on Ubuntu 16.04 x86.
I can confirm this issue is fixed, I verified using Firefox 63.0b5 on Ubuntu 16.04 x86, Windows  10 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: