1104672 - Highlighter nodeinfobar isn't fully visible when classes and/or id are long and the element is near edge of page.

Status: RESOLVED DUPLICATE of bug 1349275

(DevTools :: Inspector, defect, P3)

Description

[kud]

Attachment: tooltip z-index bug

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141124030207

Steps to reproduce:

See the screenshot.



Expected results:

The tooltip should be over the devtools window.

Comment 1

[Patrick Brosset <:pbro>]

I'm afraid this isn't as simple as a z-index problem. The tooltip (or nodeinfobar) is actually part of the content page. It is inserted in the canvasframe (one of the root frame elements of the page) as native anonymous content.
This was done so that the highlighter would work the same on all devices running gecko.
But because of this, it isn't like other XUL panel-based tooltips we have, it does not float on top of the window.

What we should do here is make sure it is fully visibly though, by pushing it to the left if the element it points to is too close to the right edge of the viewport.
We already have logic to handle these cases, so the fix should be somewhere in the _moveInfobar function in http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#1265

Comment 2

[Patrick Brosset <:pbro>]

STR:

- Open the test URL
- Open the inspector
- Select the only DIV in the page

==> The node infobar is only partly shown

Comment 3

[Patrick Brosset <:pbro>]

A fixed buffer is used here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#1301
This wasn't the case previously, we used to calculate the width of the nodeinfobar so as to move it by the right amount.
Because of the way the nodeinfobar is inserted in the page, we can't calculate its width anymore, and so a fixed buffer was introduced instead. Obviously, for anything larger than 100px, this approach fails.

Several solutions:
- add an ellipsis to the nodeinfobar to restrict its size so it always fits in the buffer
- find another way to dynamically calculate the width of the infobar: maybe just counting the number of character, multiplying by the character width, and adding a buffer for the left/width padding.

Comment 4

[Patrick Brosset <:pbro>]

As discussed on IRC, assigning you the bug Erwann.
Feel free to ask on IRC #devtools for more info about the dev environment and the code.

Comment 5

[kud]

Perfect. I will try to do something as possible as I can. ;)

Thank you.

Comment 6

[Josh Matthews [:jdm]]

Are you still working on this, Erwann?

Comment 7

[kud]

For the moment no unfortunately.

Comment 8

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

I add a fix for this bug in the patch of bug 1349275; so it shouldn't be happens anymore.
We should have the QA testing this once the patch is landed.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> I add a fix for this bug in the patch of bug 1349275; so it shouldn't be
> happens anymore.
> We should have the QA testing this once the patch is landed.

How should we get the QA testing here?  Neither bug is marked qe-verify+ for the moment.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> > I add a fix for this bug in the patch of bug 1349275; so it shouldn't be
> > happens anymore.
> > We should have the QA testing this once the patch is landed.
> 
> How should we get the QA testing here?  Neither bug is marked qe-verify+ for
> the moment.

I'm actually waiting for the review; if there is no substantial change to the patch, then I'll set qe-verify+.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> > (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> > > I add a fix for this bug in the patch of bug 1349275; so it shouldn't be
> > > happens anymore.
> > > We should have the QA testing this once the patch is landed.
> > 
> > How should we get the QA testing here?  Neither bug is marked qe-verify+ for
> > the moment.
> 
> I'm actually waiting for the review; if there is no substantial change to
> the patch, then I'll set qe-verify+.

Hmm, I don't think I follow your comment.  Bug 1349275 has already landed, and there are no open review requests.  Is there a different bug that you are thinking of?

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)

> > I'm actually waiting for the review; if there is no substantial change to
> > the patch, then I'll set qe-verify+.

> Hmm, I don't think I follow your comment.  Bug 1349275 has already landed,
> and there are no open review requests.  Is there a different bug that you
> are thinking of?

I was, indeed, thinking to another bug. :) My apologies!

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

See comment 2 about the STR for manual test the issue. It shouldn't happen anymore now that Bug 1349275 has landed.