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)
DevTools
Inspector
Tracking
(firefox63 verified)
VERIFIED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | verified |
People
(Reporter: jdescottes, Assigned: ladybenko)
References
Details
Attachments
(1 file)
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.
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(balbeza)
Reporter | ||
Comment 1•6 years ago
|
||
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®exp=false&path=browser_markup_shadow.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
Flags: needinfo?(balbeza)
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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+
Comment 6•6 years ago
|
||
(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)
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
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 8•6 years ago
|
||
mozreview-review |
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-
Updated•6 years ago
|
Flags: needinfo?(axel)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
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!
Assignee | ||
Comment 11•6 years ago
|
||
Axel, are you OK with this not being localized? Thanks :)
Flags: needinfo?(l10n)
Comment 12•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
Pushed by balbeza@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/98f850870874 show shadow root mode. r=bgrins
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/98f850870874
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Reporter | ||
Updated•6 years ago
|
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
Comment 21•6 years ago
|
||
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
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
You need to log in
before you can comment on or make changes to this bug.
Description
•