Closed Bug 1102269 Opened 7 years ago Closed 5 years ago

[highlighter] infobar can be outside visible area

Categories

(DevTools :: Inspector, defect, P2)

defect

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
No longer blocks: 985597
Depends on: 985597
Filter on CLIMBING SHOES
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]
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]
Attachment #8747541 - Flags: review?(zer0)
Attachment #8747540 - Attachment is obsolete: true
It's quite a simple fix, but it solves the issue.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
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 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-
Assignee: philipp → nobody
Status: ASSIGNED → NEW
Assignee: nobody → rchien
Status: NEW → ASSIGNED
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)
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!
(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)
Thank you Tim. It works!
Flags: needinfo?(zer0)
Flags: needinfo?(rchien)
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)
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/
Attachment #8747541 - Attachment is obsolete: true
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"
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)
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/
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
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/
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.
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)
(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.
Comment on attachment 8777221 [details]
Bug 1102269 - Fixed infobar getting outside visible area

https://reviewboard.mozilla.org/r/68822/#review66706
Attachment #8777221 - Flags: review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/42be3a4501f9
Fixed infobar getting outside visible area r=ntim
https://hg.mozilla.org/mozilla-central/rev/42be3a4501f9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
No longer blocks: top-inspector-bugs
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]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.