1003569 - Prevent NodeInfoBar appearing outside viewport

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Dennis Schubert [:denschub]]

Attachment: Screenshot of a tooltip outside the browser content

When inspecting an element outside the current viewport, its tooltip is placed between the tabbar and the tabs on the upper browser frame, so the tooltips are visible outside the viewport. See the attached screenshot for clarification.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I can't reproduce this. Dennis, can you see if you can reproduce this on the latest nightly?
http://nightly.mozilla.org/

Comment 2

[Joe Walker [:jwalker]]

I managed to reproduce this on Nightly by highlighting using the mark-up view in a window with few tabs open.

Also +1000 for a screenshot which shows both demonstration and creation of the bug.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: RoamingTooltips.mp4

Video with steps to reproduce.

This is one of those things easier to explain using a video than a screenshot but I still claim the +1000 points.

Basically:
- Right click an element to inspect it.
- Scroll the page so that the infobar would appear close to the top of the browser XUL.
- Hover the node in the markup view.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: hide-infobar-when-outside-viewport-1003569.patch

We now display the nodeinfobar at the top left of the viewport whenever it would have been displayed above the top of the viewport.

Comment 5

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> This is one of those things easier to explain using a video than a
> screenshot but I still claim the +1000 points.

No way! Dennis got there before you, plus his image was in the creation of the bug.

Good work on a fast fix though.

The same goes for the Find bar however. So we need to fix the bottom too.
When something scrolls off the page to the left or right, we still show the infobar. Maybe we should do that for the top/bottom?

Could you ask Patrick for review plz.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Show in correct position

Now we show the tooltip:
1. At the top of the content area when a node is above the content area.
2. At the bottom of the content area above the findbar when a node is below the content area and the findbar is visible.
3. At the bottom of the content area when a node is below the content area and the findbar is not visible.

Comment 7

[Patrick Brosset <:pbro>]

I applied the patch and played with scrolling.
The problem described in this bug seems to be gone. Nice!

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8415319 [details] [diff] [review]
Show in correct position

Review of attachment 8415319 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good to me overall.
I just have a doubt about accessing gBrowser.getFindBar() (see my comment below).
Also, we should try to get a test in for this.

::: toolkit/devtools/server/actors/highlighter.js
@@ +737,5 @@
>      if (this.rect) {
>        let bounds = this.rect.bounds;
>        let winHeight = this.win.innerHeight * this.zoom;
>        let winWidth = this.win.innerWidth * this.zoom;
> +      let findbar = this.chromeWin.gBrowser.getFindBar();

I'm unsure whether this will work on all debugger targets.
I know we're not using the BoxModelHighlighter on B2G yet, but we will at some stage and I don't know how this will work then.
Is gBrowser going to be available?
Is getFindBar defined on it, on all targets?
I don't know how to answer these questions, perhaps you should ping paul about this.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: hide-infobar-when-outside-viewport-1003569.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Comment on attachment 8415319 [details] [diff] [review]
> Show in correct position
> 
> Review of attachment 8415319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good to me overall.
> I just have a doubt about accessing gBrowser.getFindBar() (see my comment
> below).
> Also, we should try to get a test in for this.
> 
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +737,5 @@
> >      if (this.rect) {
> >        let bounds = this.rect.bounds;
> >        let winHeight = this.win.innerHeight * this.zoom;
> >        let winWidth = this.win.innerWidth * this.zoom;
> > +      let findbar = this.chromeWin.gBrowser.getFindBar();
> 
> I'm unsure whether this will work on all debugger targets.
> I know we're not using the BoxModelHighlighter on B2G yet, but we will at
> some stage and I don't know how this will work then.
> Is gBrowser going to be available?
> Is getFindBar defined on it, on all targets?
> I don't know how to answer these questions, perhaps you should ping paul
> about this.

Good points. On B2G gBrowser is not available and getFindbar is Desktop only. 

Wherever gBrowser is available getFindbar is too so I have added a condition for that block:
if (this.chromeWin.gBrowser) {
  let browserTop = this.browser.getBoundingClientRect().top;
  let findbar = this.chromeWin.gBrowser.getFindBar();
  let findTop = findbar.getBoundingClientRect().top - browserTop;

  ptop = Math.min(ptop, findTop);
}

Of course, to truly make the box model highlighter compatible with B2G we will need to make a bunch of changes beginning with obtaining a layer that we can actually draw on.

I will add a test in a separate patch.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8434823 [details] [diff] [review]
hide-infobar-when-outside-viewport-1003569.patch

Review of attachment 8434823 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a few minor changes to make the code clearer.

::: toolkit/devtools/server/actors/highlighter.js
@@ +742,5 @@
>    /**
>     * Move the Infobar to the right place in the highlighter.
>     */
>    _moveInfobar: function() {
>      if (this.rect) {

not a big deal since this code was like this before, but it would look better if written like this:

if (!this.rect) {
  this.nodeInfo.positioner.style.left = "0";
  ...
  return;
}

let bounds = this.rect.bounds;
...

@@ +747,5 @@
>        let bounds = this.rect.bounds;
>        let winHeight = this.win.innerHeight * this.zoom;
>        let winWidth = this.win.innerWidth * this.zoom;
> +      let pbottom = Math.max(0, bounds.bottom);
> +      let ptop = Math.max(0, bounds.top);

why the 'p' prefix and no camelcase? Can you find a more explicit name for these variables?

@@ +754,5 @@
> +        let browserTop = this.browser.getBoundingClientRect().top;
> +        let findbar = this.chromeWin.gBrowser.getFindBar();
> +        let findTop = findbar.getBoundingClientRect().top - browserTop;
> +
> +        ptop = Math.min(ptop, findTop);

I honestly have a little bit of a hard time understanding this code.
I've applied the patch and tested it, it seems to work fine, so I'm sure this code is alright, but I would advise adding at least a 1 line comment explaining what is done here, and why it is done.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: hide-infobar-when-outside-viewport-1003569.patch

Rebased

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/9215a037c833

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9215a037c833