Closed
Bug 1102269
Opened 10 years ago
Closed 8 years ago
[highlighter] infobar can be outside visible area
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(firefox51 verified)
VERIFIED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | verified |
People
(Reporter: ntim, Assigned: rickychien, Mentored)
References
Details
(Whiteboard: [btpp-fix-later][good taipei bug])
Attachments
(1 file, 2 obsolete files)
A regression of bug 985597 I think. STR : - Go to a long scrollable webpage (bug 985597 for example) - Don't scroll down, inspect <body> - See that the infobar is there (expected) - Scroll down - Hover <body> in the markup view - The infobar is no longer visible in the content area
Reporter | ||
Updated•10 years ago
|
Comment 1•8 years ago
|
||
Filter on CLIMBING SHOES
Comment 2•8 years ago
|
||
I was going to set P3 as priority – and maybe we'll change that later anyway – but after a consideration, I decided to set P2. This is an important visual regression IMVHO, considering the parity with the other browsers' devtools too.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Updated•8 years ago
|
Blocks: top-inspector-bugs
Comment 3•8 years ago
|
||
Matteo, I added you as a mentor for this one, feel free to unassign you if needed, but I think you might be the best one to help people fix this.
Mentor: zer0
Whiteboard: [btpp-fix-later] → [btpp-fix-later][good taipei bug]
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Updated•8 years ago
|
Attachment #8747541 -
Flags: review?(zer0)
Updated•8 years ago
|
Attachment #8747540 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
It's quite a simple fix, but it solves the issue.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8747541 [details] [diff] [review] Fixed infobar getting outside visible area Review of attachment 8747541 [details] [diff] [review]: ----------------------------------------------------------------- (Small comment, Matteo will review this) It would be nice to have a test for this, so we don't regress this in the future. You can probably base your test on those located in devtools/client/inspector/test (notably those named browser_inspector_highlighter-*.js).
Comment 8•8 years ago
|
||
Comment on attachment 8747541 [details] [diff] [review] Fixed infobar getting outside visible area Review of attachment 8747541 [details] [diff] [review]: ----------------------------------------------------------------- My deeply apologies for this huge delay; the patch looks good and it's simple enough – I think it's need a rebasing now. I agree with Tim that would be nice have a simple test to avoid regression; you could probably add a new `devtools/client/inspector/test/browser_inspector_infobar_03.js` file for that. So, the code itself is r+, I just set r- because I'd like to land this code with test already, if possible. Thanks!
Attachment #8747541 -
Flags: review?(zer0) → review-
Reporter | ||
Updated•8 years ago
|
Assignee: philipp → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•8 years ago
|
||
Hi Matteo, I'm not familiar with devtools browser tests. Is there any good way to detect the top of box-model-nodeinfobar-container with specifying a scroll position?
Flags: needinfo?(zer0)
Assignee | ||
Comment 10•8 years ago
|
||
Let me clarify my question. I'd like to access my test webpage's content itself in order to set window.scrollTo() to scroll down entire page. It's not easy for me to figure out how to do this. Any help would be appreciated!
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #10) > Let me clarify my question. I'd like to access my test webpage's content > itself in order to set window.scrollTo() to scroll down entire page. > > It's not easy for me to figure out how to do this. Any help would be > appreciated! Does this work ? yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => { content.scrollTo(...); });
Flags: needinfo?(rchien)
Assignee | ||
Comment 12•8 years ago
|
||
Thank you Tim. It works!
Flags: needinfo?(zer0)
Flags: needinfo?(rchien)
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68822/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68822/
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68822/diff/1-2/
Attachment #8777221 -
Flags: review?(zer0)
Assignee | ||
Comment 15•8 years ago
|
||
Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8539ddb4faf3
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68822/diff/2-3/
Assignee | ||
Comment 17•8 years ago
|
||
Fixed test failures and push to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ce06486d30
Reporter | ||
Updated•8 years ago
|
Attachment #8747541 -
Attachment is obsolete: true
Reporter | ||
Comment 18•8 years ago
|
||
https://reviewboard.mozilla.org/r/68822/#review65972 ::: devtools/client/inspector/test/browser_inspector_infobar_03.js:7 (Diff revision 3) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +// Check the position of the highlighter nodeinfo bar in scrollable webpage. Maybe this could use more precision (or at least the bug number for clarity): // Bug 1102269 - Make sure info-bar never gets outside of visible area after scrolling ::: devtools/client/inspector/test/browser_inspector_infobar_03.js:23 (Diff revision 3) > + style: "top:0px", > + }, > + ]; > + > + for (let currTest of testData) { > + yield testPositionAndStyle(currTest, inspector, testActor); Not sure why a loop is needed here. Were you planning to add more test cases? ::: devtools/client/inspector/test/browser_inspector_infobar_03.js:35 (Diff revision 3) > + yield selectAndHighlightNode(test.selector, inspector); > + > + let style = yield testActor.getHighlighterNodeAttribute( > + "box-model-nodeinfobar-container", "style"); > + > + is(style.split(";")[0], test.style, "Node " + test.selector + ": style matches"); It would be more clear if the comment was actually relating to the test: "Infobar shows on top of the page when page isn't scrolled" or something like that. ::: devtools/client/inspector/test/browser_inspector_infobar_03.js:44 (Diff revision 3) > + }); > + > + style = yield testActor.getHighlighterNodeAttribute( > + "box-model-nodeinfobar-container", "style"); > + > + is(style.split(";")[0], test.style, "Node " + test.selector + ": style matches"); Same here: "Infobar shows on top of the page even if the page is scrolled"
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68822/diff/3-4/
Attachment #8777221 -
Flags: review?(zer0) → review?(ntim.bugs)
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68822/diff/4-5/
Assignee | ||
Comment 21•8 years ago
|
||
Tim, thank you for this suggestion! I prefer to set you as reviewer for this patch. push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e9c69ee6057
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68822/diff/5-6/
Assignee | ||
Comment 23•8 years ago
|
||
After discussion with zer0 on IRC, the best way to wait for scroll event is using testActor.scrollWindow() yield testActor.scrollWindow(0, 500); At the latest patch, test case will fail normally if we unapply changes and it passed as expected if we apply the changes back.
Reporter | ||
Comment 24•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area https://reviewboard.mozilla.org/r/68822/#review66258 Everything looks fine, but I'd like to see the issue fixed.
Attachment #8777221 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 25•8 years ago
|
||
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #24) > Comment on attachment 8777221 [details] > Bug 1102269 - Fixed infobar getting outside visible area > > https://reviewboard.mozilla.org/r/68822/#review66258 > > Everything looks fine, but I'd like to see the issue fixed. Oh wait, didn't see your last comment, sorry.
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8777221 [details] Bug 1102269 - Fixed infobar getting outside visible area https://reviewboard.mozilla.org/r/68822/#review66706
Attachment #8777221 -
Flags: review+
Comment 27•8 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/42be3a4501f9 Fixed infobar getting outside visible area r=ntim
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42be3a4501f9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
No longer blocks: top-inspector-bugs
Comment 29•8 years ago
|
||
i have reproduced this bug with Nightly 36.0a1 (2014-11-20) on Windows 7, 32 Bit ! This bug's fix is verified with latest Developer Edition (Aurora) Build ID 20160923004002 User Agent Mozilla/5.0 (Windows NT 6.1; rv:51.0) Gecko/20100101 Firefox/51.0 [bugday-20160921]
Reporter | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•