Closed
Bug 968316
Opened 11 years ago
Closed 11 years ago
Highlighter: keep highlight overlay visible while selected node is hovered in markup view
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox29 fixed, firefox30 fixed)
RESOLVED
FIXED
Firefox 30
People
(Reporter: bgrins, Assigned: pbro)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first verify])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
pbro
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This is a minor polish thing I've noticed with the highlighter. STR: * Click on an element in the markup view to select an element * Notice that the overlay highlighting the element on the page appears as you hover the node * Keep your mouse still, on top of the newly selected node that was just clicked in the markup view Expected: As long as my mouse is above the element, the overlay will never disappear. Actual The overlay disappears after a couple of seconds. In order to get it to show and stay back up, I must move the mouse off of the element and back onto it.
Assignee | ||
Comment 1•11 years ago
|
||
Clicking on an element in the markup-view or in the breadcrumbs selects it and triggers a brieflyShowBoxModel, which uses a timeout to unhighlight after 1 second. On the other hand, we also highlight/unhighlight on mousemove of the markup-view. So what's happening here is that even if the mousemove event on the markup-view is detected and used to highlight the node, the 1 second timeout is still running from having selected the element before, and when it expires, will hide the highlighter, even if the mouse is still over the markup-view. That should be an easy fix. I haven't thought of it much, but it should be possible to clear the timeout in the highlighter actor if we receive a request to show the highlighter again after it's started.
Assignee | ||
Comment 2•11 years ago
|
||
Actually I was wrong, the fix should be client side since the events are going to be triggered in the opposite order: first the node gets hovered over (highlighter shown) and then clicked (highlighter shown then hidden after 1 sec). Therefore the fix is to not call brieflyShowBoxModel if the node is already being hovered over.
Assignee: nobody → pbrosset
Assignee | ||
Comment 3•11 years ago
|
||
This should do it. Ongoing try build https://tbpl.mozilla.org/?tree=Try&rev=886096ddb1dd The patch contains: - a simple change to markup-view's _shouldNewSelectionBeHighlighted that checks if the node isn't already hovered when it gets selected - a new test - some refactoring of the markup-view's test head.js file
Attachment #8371013 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 4•11 years ago
|
||
The patch definitely fixes the reported issue - I'm still looking at the code. There is another issue I've just noticed (with and without the patch applied). Select a node, then mouse out of the markup view (while keeping the mouse over the selected node before leaving). Then mouse back into the markupview on the selected node, and the highlight will not show back up. This is really a pretty minor polish thing, and this fix should be good to go even without that. I mention it here because if it is something closely related it may be worth adding it.
Assignee | ||
Comment 5•11 years ago
|
||
Definitely. Good catch. I'll investigate and add a fix for this here too.
Assignee | ||
Comment 6•11 years ago
|
||
New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=4ba791d8238a This fixes the minor problem you found in your last comment and adds a new test for it.
Attachment #8371013 -
Attachment is obsolete: true
Attachment #8371013 -
Flags: review?(bgrinstead)
Attachment #8371342 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 8371342 [details] [diff] [review] bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v2.patch Review of attachment 8371342 [details] [diff] [review]: ----------------------------------------------------------------- This looks good ::: browser/devtools/markupview/test/browser_inspector_markup_968316_highlight_node_after_mouseleave_mousemove.js @@ +19,5 @@ > +} > + > +function startTests(aInspector, aToolbox) { > + let p = content.document.querySelector("p"); > + Task.spawn(function() { I like this format for the test - very readable and no globals to clean up @@ +40,5 @@ > + ok(isHighlighterVisible(), "the highlighter is visible again"); > + }).then(null, ok.bind(null, false)).then(endTests); > +} > + > +function waitFor(ms) { This function isn't being used in the test ::: browser/devtools/markupview/test/browser_inspector_markup_968316_highlit_node_on_hover_then_select.js @@ +46,5 @@ > +} > + > +function waitForTheBrieflyShowBoxModelTimeout() { > + let deferred = promise.defer(); > + // Note that the current timeout is 1 sec and is neither configurable nor We should definitely have this constant exported at some point... I can just imagine compatibility issues we run when we decide to switch that to 2 seconds or something and are running tests against old servers. In this case it doesn't really matter though, because we could always make the test wait longer and it would still work.
Attachment #8371342 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Thanks for the quick review. Sorry about the extra function, I was just experimenting and forgot to remove it.
Assignee | ||
Comment 9•11 years ago
|
||
Just removed the extra function.
Attachment #8371342 -
Attachment is obsolete: true
Attachment #8371467 -
Flags: review+
Assignee | ||
Comment 10•11 years ago
|
||
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/8c2a612f3d15
Whiteboard: [fixed-in-fx-team]
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c2a612f3d15
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8371467 [details] [diff] [review] bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v3.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): In Firefox 29, we refactored the way the devtools highlighter generally works. The interactions are different now, as seen here https://hacks.mozilla.org/2014/01/upcoming-changes-to-the-firefox-developer-tools-node-picker/ (done in bug 916443). After that bug landed, a few follow-up bugs were fixed, including this one, which fixes the way the highlighter appears when hovering over the markup-view. User impact if declined: If declined, users may not always see the highlighter when hovering over nodes in the markup-view, which could be frustrating especially with the new way the highlighter behaves. Also, I'm about to request uplift of bug 962647 too, and this one is a dependency of bug 962647. Testing completed (on m-c, etc.): Tested on mozilla-central. It's been in nightly for several weeks. New browser mochitest added and passing. Risk to taking this patch (and alternatives if risky): The patch consists of lines changes in markup-view.js (the rest is tests), and looking at the code, I don't see risks involved with applying this to Aurora. String or IDL/UUID changes made by this patch: None
Attachment #8371467 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•11 years ago
|
Attachment #8371467 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Whiteboard: [good first verify]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•