STR: 1. Go to https://www.mozilla.org/en-US/ 2. Enable node picker 3. Hover on an element down the page, such as the text "Stand Up for Encryption" ER: The node dimensions tooltip should appear nearby the highlighted content AR: It's quite far away vertically, and many times it is positioned outside of the current viewport, so you'd have to scroll to find it. :zer0 suspected on IRC this may be related to APZ scrolling issues.

You can see here that the tooltip is way too high, and it was off screen when I first picked the element.

:zer0, you mentioned you had some ideas about this.

[Tracking Requested - why for this release]: Usability regression in DevTools inspector UI.

That's definitely a regression from Bug 1312103. The `moveInfobar` function wasn't updated accordingly. Taking this bug.

So, the `moveInfobar` was still doing the math thinking in term of fixed viewport, where now both box-model and css-grid are positioned absolutely. Basically, we had to add the scrolling values during our calculation. That's easy to fix. However, that is not all the story, and it's not enough. Previously, we move / redraw the highlighter based on the document's scrolling: AutoRefreshHighlighter was calling `update` at each scroll, with two results: the first one, we could update the infobar's position. The second one, we got latency due the APZ. Now we don't `update` our highlighter when we scroll, since the highlighters have `position: absolute`, that's also is good in term of performance, plus we do not get latency during the scrolling. As downside, we can't re-position properly the infobar when we scroll, even if the math is right, since we don't call `moveInfobar` anymore in such situation. We need to restore the capability for the `AutoRefreshHighlighter` to react to scrolling as well, limiting this to update the `infobar`. Unfortunately that means that the `infobar` will suffer a bit from the APZ latency again. I'm currently working on this, a patch should be ready today.

An important note: This patch doesn't change anything on Grid highlighter, but fixes the infobar used in BoxModel. I was thinking to file a new bug for the Grid – but probably bug 1348919 can be used for that. The patch took more time that initially I expected, for the several reasons, but there are two points that could be useful to know for future reference: 1. As said, in order to position correctly the infobar we have to listen to the scroll, that means we're bringing back the APZ latency issue (See bug 1312103); that would be especially visible if we scroll for a while when the element inspected is offscreen: we have to reposition at each "scroll" the infobar on the margins of the viewport. To minimize this issue, I decided to implement a sort of version of `position: sticky`, but instead of behave like `position: relative` and then `position: fixed` when reach the boundaries, it's a `position: absolute` and then a `position: fixed`. This helps a lot with the APZ latency; unfortunately `position: fixed` and `transform` doesn't work well together (since any value other than `none` for the transform will basically "cancel" the `position: fixed`), and we use the `transform` to scale the highlighters when we zoom. In order to make everything works without losing the advantage over the APZ latency given by the position fixed, I had to scale separately the infobar and the highlighter (also for that reason the inforbar math and style now is different). Therefore the box-model highlighter now scales `elements` instead of the `root`: it's a good approach anyway, in future and current highlighter we should have the infobar(s) separated by the rest of the highlighter's content. 2. With this patch we should solve also other infobar bugs (e.g. from time to time, the infobar could be partially hidden horizontally, if long enough). I don't recall a bugs for that but if there is we can fix as duplicated. I was able to fix those behaviors properly because recently landed Bug 1346623, that implemented `GetComputedStylePropertyValue` for AC. That gives to use the power to do the proper math for alignment instead of guessing or rely on constants' values that are not always true. We should probably keep in mind this method when we're going to refactored highlighters or creating new. In addition to get the computed style from the element, now we can also avoid to duplicate variables in both CSS and JS; we can just add a CSS variable then we use in both places (in this patch I've use it for the arrow's size). Overall, the refactored infobar feels more consistent and more solid, but it needs a lot of manual tests from QA in order to be sure we don't have regressions.

If you land this please request uplift to 54. It's a bit late to bring it to 53 though.

Comment on attachment 8850862 [details] Bug 1349275 - refactored `moveInfobar` function; https://reviewboard.mozilla.org/r/123380/#review126722 Tested locally, works great for me! Also, the code changes read nicely, so I don't have a problem with this patch. Now that you have touched the nodeinfobar positioning function a lot, maybe this could give you ideas about how bug 1104672 could be solved? ::: devtools/server/actors/highlighters/auto-refresh.js:196 (Diff revision 1) > this._updateAdjustedQuads(); > > return areQuadsDifferent(oldQuads, this.currentQuads, getCurrentZoom(this.win)); > }, > > + _hasWindowScrolled: function () { Please add a jsdoc that explains what this function does and returns. In particular, it would be useful to say how often this function is called. ::: devtools/server/actors/highlighters/auto-refresh.js:253 (Diff revision 1) > // this.currentNode > // This is called as a result of a page scroll, zoom or repaint > throw new Error("Custom highlighter class had to implement _update method"); > }, > > + _scrollUpdate: function () {}, If this is supposed to be implemented by sub classes, please add a comment like in `_update`

Comment on attachment 8850862 [details] Bug 1349275 - refactored `moveInfobar` function; https://reviewboard.mozilla.org/r/123380/#review126722 That's what I was reffering in comment 7, point 2. I'll resolve bug 1104672 as duplicate. We could probably file a new bug about the arrow, hoever. Currently we display the arrow always in the center of the infobar, and if we can't position the arrow in the center of the element we just hide it. We could try at least to check if it's possible to position the arrow in a different position in the infobar, if the element's center is visible in the viewport. > If this is supposed to be implemented by sub classes, please add a comment like in `_update` I added a comment slightly different than `_update`. The difference here is that `_scrollUpdate` is not mandatory as `_update`.

Pushed by mferretti@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/448adfbcfda1 refactored `moveInfobar` function; r=pbro

sorry had to back this out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=87122717&repo=autoland&lineNumber=4280 https://hg.mozilla.org/integration/autoland/rev/c14b85ea2139

Pushed by mferretti@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f36863de500a refactored `moveInfobar` function; r=pbro

(In reply to Iris Hsiao [:ihsiao] from comment #14) > sorry had to back this out for eslint failure like > https://treeherder.mozilla.org/logviewer. > html#?job_id=87122717&repo=autoland&lineNumber=4280 > > https://hg.mozilla.org/integration/autoland/rev/c14b85ea2139 My bad, I fixed that in the follow-up bug (bug 1345434) but I didn't update this. I cherry picked the fix and upload it; now it should be fine.

Comment on attachment 8850862 [details] Bug 1349275 - refactored `moveInfobar` function; Approval Request Comment [Feature/Bug causing the regression]: Bug 1312103, the info bar position was not updated there. [User impact if declined]: If declined, the info bar that labels nodes in DevTools can easily get lost on screen and will be in the wrong position. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: DevTools inspector only, changes positioning of UI elements [String changes made/needed]: None

Comment on attachment 8850862 [details] Bug 1349275 - refactored `moveInfobar` function; Regression from 53, let's uplift the fix.

Backed out for devtools test failures. https://treeherder.mozilla.org/logviewer.html#?job_id=88105452&repo=mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/260ad3e3f8970a637c8acef0b83fde9da19d1044 https://hg.mozilla.org/releases/mozilla-beta/rev/4c6f403862d369eb0f3f9eff423c6458f36ec4bf

Bogdan, could you please take a look at this on Beta 53? Instructions in Comment 0.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24) > Backed out for devtools test failures. > https://treeherder.mozilla.org/logviewer.html#?job_id=88105452&repo=mozilla- > aurora > > https://hg.mozilla.org/releases/mozilla-aurora/rev/ > 260ad3e3f8970a637c8acef0b83fde9da19d1044 > https://hg.mozilla.org/releases/mozilla-beta/rev/ > 4c6f403862d369eb0f3f9eff423c6458f36ec4bf This patch uses the code implemented in Bug 1346623 as mentioned in comment 7, so it can't be uplifted without uplifting bug 1346623 upfront. My apologies, I thought it was listed in the uplift request, but I notice it wasn't.

I actually tried to request the uplift, with the proper list of other uplifts needed, but it seems I can't.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #26) > (In reply to Ryan VanderMeulen [:RyanVM] from comment #24) > > Backed out for devtools test failures. > > https://treeherder.mozilla.org/logviewer.html#?job_id=88105452&repo=mozilla- > > aurora > > > > https://hg.mozilla.org/releases/mozilla-aurora/rev/ > > 260ad3e3f8970a637c8acef0b83fde9da19d1044 > > https://hg.mozilla.org/releases/mozilla-beta/rev/ > > 4c6f403862d369eb0f3f9eff423c6458f36ec4bf > > This patch uses the code implemented in Bug 1346623 as mentioned in comment > 7, so it can't be uplifted without uplifting bug 1346623 upfront. My > apologies, I thought it was listed in the uplift request, but I notice it > wasn't. My mistake, I missed the dependency when writing the uplift requests. :( I guess we also need approved uplift requests on the other bug? :zer0, do you want to add them? I'm worried I'll miss other details now...

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28) > I guess we also need approved uplift requests on the other bug? :zer0, do > you want to add them? I'm worried I'll miss other details now... To play safe, I just asked in the other bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1346623#c23); since I didn't work on it, I could easily miss other details as well.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28) > My mistake, I missed the dependency when writing the uplift requests. :( No worries! :)

Patrick, Matteo, this has missed getting into 53 but we could still consider taking it on 54 aurora this week if you can straighten out which patches you need. This late in aurora we should also ask QE directly for testing help. How bad is it to ship 53 with this regression?

This is a very user-facing thing, we should try and take it on 54. Matteo, do we have a good understanding of the dependencies needed to make this happen?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32) > Patrick, Matteo, this has missed getting into 53 but we could still consider > taking it on 54 aurora this week if you can straighten out which patches you > need. This late in aurora we should also ask QE directly for testing help. We need to uplift the part 2 of bug 1346623 (https://reviewboard.mozilla.org/r/121066/diff/#index_header), according with the author of the patch should be fine just this. Then we can uplift this patch on top of that. > How bad is it to ship 53 with this regression? As Patrick said, it's very user-facing, the infobar will be basically hidden when the user will scroll the page, instead of being properly displayed.

Liz, Ryan, we should try to uplift this patch as soon as part 2 of bug 1346623 get uplifted – see my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1346623#c29 As far as I understand from pbro, we've still a chance to get this into 53.

I think we may need to live with this one for 53. pbro is saying it is not a release blocker. We are so late in the release cycle that uplifting this potentially could delay the release. Sorry this missed the train.

I reproduced this issue using Fx 55.0a1, build ID: 20170321030211, on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 55.0a1, build ID: 20170423030206 and Fx 54.0a2, build ID: 20170418004027, on Windows 10 x64, mac OS X 10.12.3 and Ubuntu 14.04 LTS. Cheers!