Closed
Bug 1456680
Opened 6 years ago
Closed 6 years ago
Toggle the grid highlighter from the markup badges
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
Tracking
(firefox63 fixed)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: gl, Assigned: gl)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
pbro
:
review+
|
Details |
59 bytes,
patch
|
pbro
:
review+
gl
:
checkin+
|
Details | Diff | Splinter Review |
59 bytes,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
59 bytes,
patch
|
pbro
:
review+
gl
:
checkin+
|
Details | Diff | Splinter Review |
Bug 1456680 - Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay.
59 bytes,
patch
|
pbro
:
review+
gl
:
checkin+
|
Details | Diff | Splinter Review |
59 bytes,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8971108 [details] [diff] [review] Bug 1456680 - Part 4: Rename event and display node to badge in element-editor.js; https://reviewboard.mozilla.org/r/239906/#review245720 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/element-editor.js:282 (Diff revision 1) > - // Update the display type node > - let showDisplayNode = this.node.displayType in DISPLAY_TYPES; > - this.displayNode.textContent = this.node.displayType; > - this.displayNode.dataset.display = this.node.displayType; > - this.displayNode.style.display = showDisplayNode ? "inline-block" : "none"; > - this.displayNode.title = showDisplayNode ? DISPLAY_TYPES[this.node.displayType] : ""; > + // Update the display markup badge > + let showDisplayBadge = this.node.displayType in DISPLAY_TYPES; > + this.displayBadge.textContent = this.node.displayType; > + this.displayBadge.dataset.display = this.node.displayType; > + this.displayBadge.style.display = showDisplayBadge ? "inline-block" : "none"; > + this.displayBadge.title = showDisplayNode ? DISPLAY_TYPES[this.node.displayType] : ""; Error: 'showdisplaynode' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8971106 [details] [diff] [review] Bug 1456680 - Part 2: Format markup.js; https://reviewboard.mozilla.org/r/239902/#review246324
Attachment #8971106 -
Flags: review?(pbrosset) → review+
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8971108 [details] [diff] [review] Bug 1456680 - Part 4: Rename event and display node to badge in element-editor.js; https://reviewboard.mozilla.org/r/239906/#review246326
Attachment #8971108 -
Flags: review?(pbrosset) → review+
Comment 20•6 years ago
|
||
I've started reviewing and testing this, but here are some comments about the feature itself: - It's a new UI feature and we're within the code freeze for 61. So let's land this after the merge so it has a chance of sitting in nightly 62 for a full cycle and be tested/fine-tuned if needed. - Isn't it going to be seen as inconsistent by users if clicking on 'grid' does something, but clicking on 'flex', 'contents', 'flow-root' doesn't? If you make one clickable, that sets the expectation that all are clickable. Pinging Victoria on this particular point: is it a problem? Do we have ideas for making the others clickable in some way?
Flags: needinfo?(victoria)
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8971917 [details] [diff] [review] Bug 1456680 - Part 5: Remove unused onShowGridLineNamesHighlight method. https://reviewboard.mozilla.org/r/240650/#review246328
Attachment #8971917 -
Flags: review?(pbrosset) → review+
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8971919 [details] [diff] [review] Bug 1456680 - Part 7: Make the grid display markup badge able to toggle on and off the grid highlighter. https://reviewboard.mozilla.org/r/240654/#review246330 ::: devtools/client/inspector/markup/views/element-editor.js:649 (Diff revision 1) > // Start listening for mutations until we find an attributes change > // that modifies this attribute. > this.markup.inspector.once("markupmutation", onMutations); > }, > > + onDisplayBadgeClick: function(event) { Can you please a simple jsdoc here?
Attachment #8971919 -
Flags: review?(pbrosset) → review+
Comment 23•6 years ago
|
||
Oh, here's another comment I thought about while testing: the tooltip shown on hover of the grid badge now also need to say what happens when users click it. It also still needs to say what grid display type means (as it does today).
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8971105 [details] Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted. https://reviewboard.mozilla.org/r/239900/#review245724 ::: devtools/client/inspector/shared/highlighters-overlay.js:147 (Diff revision 3) > } > > /** > + * Load the grid highligher display settings into the store from the stored preferences. > + */ > + loadHighlighterSettings() { The highlighter-overlay deals with more than just the grid highlighter, so now that this method lives here, it needs to have a more specific name than that. Otherwise we don't know if this is for grid, fonts, flex, something else.
Attachment #8971105 -
Flags: review?(pbrosset) → review+
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8971107 [details] [diff] [review] Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted. https://reviewboard.mozilla.org/r/239904/#review246336 ::: devtools/client/inspector/markup/markup.js:2020 (Diff revision 4) > + /** > + * Handler for "grid-highlighter-hidden" event emitted from the HighlightersOverlay. > + * Updates the markup view to show an inactive grid display badge for the given > + * nodeFront. > + * > + * @param {NodeFront} nodeFront > + * The NodeFront of the grid container element for which the grid highlighter > + * is hidden for. > + */ > + _onGridHighlighterHidden(nodeFront) { > + const container = this.getContainer(nodeFront); > + > + if (container) { > + container.update(); > + } > + > + this.highlightedGridContainer = null; > + }, > + > + /** > + * Handler for "grid-highlighter-shown" event emitted from the HighlightersOverlay. > + * Updates the markup view to show an active grid display badge for the given nodeFront. > + * > + * @param {NodeFront} nodeFront > + * The NodeFront of the grid container element for which the grid highlighter > + * is shown for. > + */ > + _onGridHighlighterShown(nodeFront) { > + const container = this.getContainer(nodeFront); > + > + // Update the last highlighted grid element container to inactivate its badge. > + if (this.highlightedGridContainer) { > + this.highlightedGridContainer.update(); > + } > + > + if (container) { > + container.update(); > + } > } Let's move all of this (+ the event listener code) to the `MarkupElementContainer` class instead. No need for this higher level class to deal with this since the only thing it wants to do is locate the containers and update them anyway. You can listen to the same events in `MarkupElementContainer` by accessing the highlighters from the parent markup-view. Then, in the `onGridHighlighterHidden` callback, you can just check if `nodeFront === this.editor.node` and early return if not, and then just call `this.update();`. Also, no need for `highlightedGridContainer` anymore I believe. You can probably easily remember if a container's grid badge was clicked in `onGridHighlighterShown`. ::: devtools/client/inspector/markup/views/element-editor.js:284 (Diff revision 4) > + if (this.highlighters.gridHighlighterShown === this.node) { > + this.displayNode.classList.add("active"); > + > + // Store the MarkupElementContainer of the highlighted grid. > + this.markup.highlightedGridContainer = this.container; > + } else { > + this.displayNode.classList.remove("active"); > + } As explained above, I believe there should be a way to get rid of `highlightedGridContainer` here. This way, you can also simplify the if/else into: ``` this.displayNode.classList.toggle("active", this.highlighters.gridHighlighterShown === this.node); ```
Attachment #8971107 -
Flags: review?(pbrosset)
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8971918 [details] [diff] [review] Bug 1456680 - Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay. https://reviewboard.mozilla.org/r/240652/#review246344 Nice simplifications overall.
Attachment #8971918 -
Flags: review?(pbrosset) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 27•6 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #20) > I've started reviewing and testing this, but here are some comments about > the feature itself: > > - It's a new UI feature and we're within the code freeze for 61. So let's > land this after the merge so it has a chance of sitting in nightly 62 for a > full cycle and be tested/fine-tuned if needed. > - Isn't it going to be seen as inconsistent by users if clicking on 'grid' > does something, but clicking on 'flex', 'contents', 'flow-root' doesn't? If > you make one clickable, that sets the expectation that all are clickable. > Pinging Victoria on this particular point: is it a problem? Do we have ideas > for making the others clickable in some way? My original thinking was that all badges should be clickable, including both grid and flex badges — I didn't know about contents and flow-root though. I can understand the reason to postpone the clickable badge feature for 62. For 61, I'd suggest we style all non-clickable badges differently from the event badges to show that they aren't active buttons yet. I'll work on the styling for this, and post a new mockup here soon.
Flags: needinfo?(victoria)
Comment 28•6 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/80f9a47393ed Part 1: Store the grid highlighter setting in HighlightersOverlay. r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/aef2d7d27ff1 Part 2: Format markup.js; r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa4b6d2be06 Part 5: Remove unused onShowGridLineNamesHighlight method. r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/232bed10f4d8 Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay. r=pbro
Assignee | ||
Updated•6 years ago
|
Attachment #8971105 -
Flags: checkin+
Assignee | ||
Updated•6 years ago
|
Attachment #8971106 -
Flags: checkin+
Assignee | ||
Updated•6 years ago
|
Attachment #8971917 -
Flags: checkin+
Assignee | ||
Updated•6 years ago
|
Attachment #8971918 -
Flags: checkin+
Comment 29•6 years ago
|
||
Backed out 4 changesets (bug 1456680) for failing dt at devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-area.js backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/7134ec98560255d2c6ffa8b47a1fd78f04d29851 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=232bed10f4d8f36466070a58ae5a37156e24d894 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=176409137&repo=mozilla-inbound&lineNumber=3725
Flags: needinfo?(gl)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gl)
Comment 30•6 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea070f2a2b31 Part 1: Store the grid highlighter setting in HighlightersOverlay. r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/81a4e70b3229 Part 2: Format markup.js; r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7bc1fadc3f Part 5: Remove unused onShowGridLineNamesHighlight method. r=pbro
Comment 31•6 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d92d104b4b5f Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay. r=pbro
Comment 32•6 years ago
|
||
I updated this badge mockup with new non-clickable styles. The [...] icons should turn into clickable badges. New changes are highlighted in purple. (Includes a change to the dark clickable hover color, since I had to shift things around a bit.) https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens There are more problems to be solved for toggle-able grid/flex icons that can show up in selected rows - I'll do that separately assuming 62 for that feature. I'm hoping this unclickable change can be added for 61.
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea070f2a2b31 https://hg.mozilla.org/mozilla-central/rev/81a4e70b3229 https://hg.mozilla.org/mozilla-central/rev/0f7bc1fadc3f https://hg.mozilla.org/mozilla-central/rev/d92d104b4b5f
Assignee | ||
Comment 34•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8971107 [details] [diff] [review] Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted. https://reviewboard.mozilla.org/r/239904/#review246336 > Let's move all of this (+ the event listener code) to the `MarkupElementContainer` class instead. No need for this higher level class to deal with this since the only thing it wants to do is locate the containers and update them anyway. > > You can listen to the same events in `MarkupElementContainer` by accessing the highlighters from the parent markup-view. > Then, in the `onGridHighlighterHidden` callback, you can just check if `nodeFront === this.editor.node` and early return if not, and then just call `this.update();`. > > Also, no need for `highlightedGridContainer` anymore I believe. You can probably easily remember if a container's grid badge was clicked in `onGridHighlighterShown`. I had to move the toggling of the "active" class to ths handler because we can't simply just check if nodeFront === this.editor.node and do an update. Since we won't clear the "active" class for the grid node that got unhighlighted, and I didn't want to do a full this.update() call since the only the displayNode "active" class is being toggled.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8971106 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971107 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971108 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971917 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971918 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971919 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8971105 -
Flags: review?(pbrosset)
Attachment #8971105 -
Flags: review+
Attachment #8971105 -
Flags: checkin+
Assignee | ||
Updated•6 years ago
|
Attachment #8971108 -
Attachment is obsolete: false
Attachment #8971108 -
Attachment is patch: true
Attachment #8971108 -
Attachment mime type: text/x-review-board-request → text/plain
Assignee | ||
Updated•6 years ago
|
Attachment #8971107 -
Attachment is obsolete: false
Attachment #8971107 -
Attachment is patch: true
Attachment #8971107 -
Attachment mime type: text/x-review-board-request → text/plain
Assignee | ||
Updated•6 years ago
|
Attachment #8971919 -
Attachment is obsolete: false
Attachment #8971919 -
Attachment is patch: true
Attachment #8971919 -
Attachment mime type: text/x-review-board-request → text/plain
Assignee | ||
Updated•6 years ago
|
Attachment #8971106 -
Attachment is obsolete: false
Attachment #8971106 -
Attachment is patch: true
Attachment #8971106 -
Attachment mime type: text/x-review-board-request → text/plain
Assignee | ||
Updated•6 years ago
|
Attachment #8971917 -
Attachment is obsolete: false
Attachment #8971917 -
Attachment is patch: true
Attachment #8971917 -
Attachment mime type: text/x-review-board-request → text/plain
Assignee | ||
Updated•6 years ago
|
Attachment #8971918 -
Attachment is obsolete: false
Attachment #8971918 -
Attachment is patch: true
Attachment #8971918 -
Attachment mime type: text/x-review-board-request → text/plain
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8971105 [details] Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted. https://reviewboard.mozilla.org/r/239900/#review258464 Found one bug: - open about:newtab - in the grid sidebar panel, click to enable the ul.section-list grid - in the same sidebar panel, click on the inspector icon next to ul.section-list - this will auto-expand the DOM tree and select the element in there The bug here is that the badge is *not* highlighted in this case. ::: devtools/client/inspector/markup/views/element-container.js:72 (Diff revision 4) > + /** > + * Handler for "grid-highlighter-hidden" and "grid-highlighter-shown" event emitted from > + * the HighlightersOverlay. Toggles the active state of the display badge if it matches > + * the highlighted grid node. > + */ > + onGridHighlighterChange: function() { About the bug I noticed, you probably just need to call this the first time the MarkupElementContainer instance is created.
Attachment #8971105 -
Flags: review?(pbrosset) → review+
Comment 37•6 years ago
|
||
Revisiting the "toggled-on badge on selected row" blue-on-blue problem: I saw a screenshot from gl of what is currently there, and it looked pretty good - 1px white border and looks like the text might be bolded? I worked on a few other variants and I think this version with black-on-light blue (#B7DAFF) background - with Blue 40 border, no bold needed - might be better in that it'll be a bit more visible against the blue row: https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens It's less strong of a toggled look for unselected rows, but seems like the row will almost always be selected when toggling this badge, since clicking to toggle will select the row (I assume?).
Flags: needinfo?(gl)
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Victoria Wang [:victoria] from comment #37) > Revisiting the "toggled-on badge on selected row" blue-on-blue problem: > > I saw a screenshot from gl of what is currently there, and it looked pretty > good - 1px white border and looks like the text might be bolded? > > I worked on a few other variants and I think this version with > black-on-light blue (#B7DAFF) background - with Blue 40 border, no bold > needed - might be better in that it'll be a bit more visible against the > blue row: > > https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens > > It's less strong of a toggled look for unselected rows, but seems like the > row will almost always be selected when toggling this badge, since clicking > to toggle will select the row (I assume?). Let's move this work onto a new bug.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gl)
Keywords: leave-open
Comment 39•6 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/215382ac0854 Part 3: Show an active state for the grid markup badge if its grid container is highlighted. r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc3dfb5191c Part 4: Rename event and display node to badge in element-editor.js; r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/332ec3041d11 Part 7: Make the grid display markup badge able to toggle on and off the grid highlighter. r=pbro
Comment 40•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/215382ac0854 https://hg.mozilla.org/mozilla-central/rev/ccc3dfb5191c https://hg.mozilla.org/mozilla-central/rev/332ec3041d11
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in
before you can comment on or make changes to this bug.
Description
•