Closed Bug 1312103 Opened 8 years ago Closed 7 years ago

Avoid scrolling latency on highlighters given by APZ

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox52 wontfix, firefox53 verified, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox53 --- verified
firefox54 --- fixed

People

(Reporter: gl, Assigned: zer0)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;
59 bytes, text/x-review-board-request
pbro
: review+
lizzard
: approval-mozilla-beta+
Details
Highlighters Stand Alone Test Page
30.95 KB, text/html
Details 
Currently, our highlighters have that lag between scrolling when we toggle the highlighter on for an element. We should disable APZ for the highlighters we have in DevTools. I would imagine we would do something similar to Bug 1250398 inside our AutoRefreshHighlighter (auto-refresh.js).
Depends on: dt-grid
Blocks: dt-grid
No longer depends on: dt-grid
@kats, I wanted to inquire if what we did in Bug 1250398 would still be effective in disabling APZ partially for certain tools. In particular, we are trying to address some APZ issues with tearing if we have a CSS grid highlighter that is toggled on and persists for the page. The CSS grid highlighter can be toggle-able when Bug 1302536 lands so we want to be able to address this. The only issue I see is that we would be adding the wheel event listener to the window and effectively hijacking the window's scroll when the highlighter is on. Are there better solutions?
Flags: needinfo?(bugmail)
I would really like to avoid doing things to disable APZ, particularly for the content window, which is what it sounds like you're proposing. In addition to it being a bad user experience, you'd also need to handle it for all input types (e.g. touch input as well as wheel input) and keep it in sync with the APZ code forever. This sounds like a pretty bad idea.

I'm not sure how the CSS grid highlighter works, but is it possible to make it APZ-friendly by using CSS properties that we support updating in the compositor?
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> I would really like to avoid doing things to disable APZ, particularly for
> the content window, which is what it sounds like you're proposing. In
> addition to it being a bad user experience, you'd also need to handle it for
> all input types (e.g. touch input as well as wheel input) and keep it in
> sync with the APZ code forever. This sounds like a pretty bad idea.
> 
> I'm not sure how the CSS grid highlighter works, but is it possible to make
> it APZ-friendly by using CSS properties that we support updating in the
> compositor?

All our highlighters uses a Canvas frame anonymous content container (see dom/webidl/Document.webidl). Inside this anonymous content we insert a canvas to rend all the lines needed for our highlighters. Perhaps we can do something to disable APZ for these anonymous content containers?
Flags: needinfo?(bugmail)
Anonymous content should already work fine with APZ. The touch selection carets are also put into anonymous content and they are positioned correctly during APZ scrolling. Can you describe a bit more the problems you are seeing that make you want to disable APZ? Or provide steps that I can use to reproduce the problem?
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Anonymous content should already work fine with APZ. The touch selection
> carets are also put into anonymous content and they are positioned correctly
> during APZ scrolling.
> Can you describe a bit more the problems you are
> seeing that make you want to disable APZ? Or provide steps that I can use to
> reproduce the problem?
Devtools highlighters are inserted into the anonymous content generated into the CanvasFrame of HTML documents, and the first thing we do to them in CSS is `position:fixed` [0].
The reason we do this is that all our highlighters cover the entire viewport, no matter if what they are actually highlighting is in an iframe, or an element with an scrolled overflow or something.
So devtools calculate the position of the highlighted element relative to the current viewport, and puts the highlighter at that position.
Now, it means that devtools does this on scroll too! And on reflow. For instance, if the currently highlighted element has a css animation that transforms it, or if the page or iframe is scrolled, then we want the highlighter to move/re-position/change shape accordingly.
Highlighters use a requestAnimationFrame to do this [1].

This explains the lag that Gabriel was talking about. If you start scrolling the page while the highlighter is present, then you can notice it lag behind instead of scrolling with the page.

To test this, you can use the following steps:
- open this page: https://fr.wikipedia.org/wiki/Louis,_Matthieu,_Joseph_et_Anna_Chedid
- open the inspector panel in devtools
- select any <p> element in the inspector
- locate the small inspector icon in the styles sidebar tab next to the p {} CSS rule. It should be right after the selector 'p' and looks like a small gray square
- click on that icon, this will "lock" multiple highlighters on the page (one on each element that matches that selector)
- now scroll around in the page and notice how the highlighters don't "stick" to their elements while scrolling

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.css#39
[1] http://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/auto-refresh.js#16
Thanks, that makes sense. Instead of using position:fixed, could you not use position:absolute and have your highlighters positioned document-relative rather than viewport relative? That would make it APZ-friendly and you wouldn't have this problem. You would still need to update their position on reflow, but not on scroll.
Well, not on scroll of the root scroll frame, but still on scroll of subframes.

The find bar highlighter overlay has the same problem. It uses position:absolute, so it works with APZ for the root scroll frame, but not for anything else.

It would be nice to have a way to associate things in the overlay with the actual scrolled elements that they're highlighting, and then have APZ do the magic to keep the two in sync.
Ah, good point. The touch carets have the same problem when they appear on content inside a scrollable div. Inside an iframe it's not a problem because the anonymous content is attached to the iframe document in that case, so it is position:absolute relative to the iframe document.

Maybe we should add the ability to attach anonymous content to a scrollframe somehow?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Ah, good point. The touch carets have the same problem when they appear on
> content inside a scrollable div. Inside an iframe it's not a problem because
> the anonymous content is attached to the iframe document in that case, so it
> is position:absolute relative to the iframe document.
Right, that's possible for the touch carets, but for the devtools highlighters and the findbar highlighters we can't really do that because they are meant to cover the whole viewport.
Sure, but it should be possible to attach multiple anonymous contents - one to the root document and one to each iframe. That may not be very efficient though, I'm not sure. As a compromise we might be able to disable APZ on scrollable divs if there is an anonymous content on the containing document. That combined with using position:absolute anonymous contents on all the affected documents should solve the problem. What do you think?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Sure, but it should be possible to attach multiple anonymous contents - one
> to the root document and one to each iframe. That may not be very efficient
> though, I'm not sure. As a compromise we might be able to disable APZ on
> scrollable divs if there is an anonymous content on the containing document.
> That combined with using position:absolute anonymous contents on all the
> affected documents should solve the problem. What do you think?

I think the proposed solution sounds promising. Needinfo'ing Markus and Patrick since I wasn't sure whom the final question was directed to.
Flags: needinfo?(pbrosset)
Flags: needinfo?(mstange)
Priority: -- → P3
My question was mostly directed at you and Patrick.
Flags: needinfo?(mstange)
Using position:absolute sounds like a good improvement. Doing this will require a bunch of fixes on our side but nothing too complex (note to self: the Rulers highlighter, and possibly others, is meant to stay fixed, so we'd need position:fixed for this one at least).

We'd still have the highlighter in the root document, and we wouldn't have to care about moving it on scroll of the root document. This would happen automatically and look good even with APZ.

Now, for nested scrollable things (e.g. iframes and divs), we'd still need the highlighter to be in the root document, because it draws things that extend all the way to the edges of the viewport. But we'd have to keep using requestAnimationFrame here to check if a scroll happened.
If we could disable APZ for these nested scrollable elements, that'd therefore be great.
Flags: needinfo?(pbrosset)
Yeah it should be possible to disable APZ for nested scrollable elements on documents which have anonymous content things.
@kats, can we figure out what bugs need to be file for the platform side and set to dependent on this bug? I know we will have things to fix on our highlighter side as well, but I would like to make sure we have the necessary bugs and assignees set to make sure we are working on resolving this problem since it might block the css grid inspector from landing.
Flags: needinfo?(bugmail)
@pbro, needinfoing you as a remind you to add the comment on what we need to do on the devtools side.
Flags: needinfo?(pbrosset)
I haven't attempted to do any of this for real yet, so take it with a grain of salt, but I do think we can make the situation better with fixes on devtools side only, even before what Kartikaya suggested in comment 14 is done. So I don't think we are blocked on anything here.

- In devtools\server\actors\highlighters.css, make .highlighter-container be position:absolute.

- Introduce .highlighter-fixed-container that is similar to .highlighter-container but that is position:fixed.

- Make the rulers and eye-dropper highlighters use .highlighter-fixed-container. Those 2 need to remain fixed on the page, while all the other ones need to scroll with the page, I think.

- Now that the highlighter is absolute, 100%x100% isn't going to do it anymore, so the width and height are going to have to be calculated and set dynamically via inline style on the highlighter container, like the findBar highlighter does it: https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/toolkit/modules/FinderHighlighter.jsm#635-653
However, that means that this will need to be done every time the window is resized.

- With that done, we have a highlighter container that has the right size and that scrolls with the content page, with APZ. We now need to make changes to how the auto-refresh base class updates highlighters. Specifically, it should not update highlighters on scroll of the root window anymore (but still do it on scroll of nested scrollable things). And it should still update highlighters when anything else happens (a reflow for instance).
To do this, the auto-refresh base class compares quads coordinates right now: https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/devtools/server/actors/highlighters/auto-refresh.js#154-158
It seems to me like we should keep this, but make it return false if the difference between the 2 quads is only the result of the page having been scrolled.

I think this should work modulo the unexpected edge cases I'm not thinking of and the tests that will need to be fixed.

@Matteo: I know you're working on the grid highlighter with Gabriel. He and I discussed about how this could be a good one for you to work on.
I think this is high priority. We've had problems with this in the past, but it particularly obvious with the grid highlighter, so we should get to this soon.
Flags: needinfo?(pbrosset) → needinfo?(zer0)
Filed bug 1316318 for the APZ side. I agree with :pbro that the devtools-side fixes will help the situation even without the APZ changes, and can be done right away.
Flags: needinfo?(bugmail)
Assignee: nobody → zer0
Flags: needinfo?(zer0)
Attachment #8825335 - Flags: review?(pbrosset)
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

https://reviewboard.mozilla.org/r/103506/#review105938

This change looks great. Thanks Matteo for working on this. I quickly tested a bunch of the highlighters. I most probably missed use cases though, so it might be good to have some qe verification on this bug, but we'll need to tell them what to test exactly.
I did find one major issue though, looks like a regression on the grid highlighter. See my comments below.
Other then this, the rest is R+. So let's have another round and try to land this quick so we have time for manual QA.

::: devtools/server/actors/highlighters.css:39
(Diff revision 4)
>    --highlighter-bubble-background-color: hsl(214, 13%, 24%);
>    --highlighter-bubble-border-color: rgba(255, 255, 255, 0.2);
>  }
>  
>  :-moz-native-anonymous .highlighter-container {
> -  position: fixed;
> +  position: absolute;

Maybe add a short comment telling people about the importance of APZ and scrolling, etc.

::: devtools/server/actors/highlighters.css:450
(Diff revision 4)
>    /* Width accounts for all color formats (hsl being the longest) */
>    --label-width: 160px;
>    --label-height: 23px;
>    --color: #e0e0e0;
>  
> -  position: absolute;
> +  position: fixed;

Doesn't the rulers also need position:fixed like this one?

::: devtools/server/actors/highlighters/auto-refresh.js:18
(Diff revision 4)
>  // Note that the order of items in this array is important because it is used
>  // for drawing the BoxModelHighlighter's path elements correctly.
>  const BOX_MODEL_REGIONS = ["margin", "border", "padding", "content"];
> +const QUADS_PROPS = ["p1", "p2", "p3", "p4", "bounds"];
> +
> +function isValueDifferent(oldValue, newValue, zoom) {

This is comparing 2 values, so maybe name it `areValuesDifferent`

::: devtools/server/actors/highlighters/auto-refresh.js:195
(Diff revision 4)
> -    return oldQuads !== newQuads;
> +    return areQuadsDifferent(oldQuads, this.currentQuads, getCurrentZoom(this.win));
> +  },
> +
> +  /**
> +   * Update the knowledge we have of the current window's dimensions and return `true`
> +   * if have change since.

typo: if they have changed since

::: devtools/server/actors/highlighters/auto-refresh.js:198
(Diff revision 4)
> +  /**
> +   * Update the knowledge we have of the current window's dimensions and return `true`
> +   * if have change since.
> +   * @return {Boolean}
> +   */
> +  _areWindowDimensionsChanged: function () {

`_haveWindowDimensionsChanged` would be more correct.

::: devtools/server/actors/highlighters/auto-refresh.js:200
(Diff revision 4)
> +   * if have change since.
> +   * @return {Boolean}
> +   */
> +  _areWindowDimensionsChanged: function () {
> +    let { width, height } = getWindowDimensions(this.win);
> +    let areChanged = (this._winDimensions.width !== width ||

similar here: `haveChanged` instead of `areChanged`

::: devtools/server/actors/highlighters/css-grid.js:464
(Diff revision 4)
> -    let height = this.win.innerHeight;
>  
>      // Resize the canvas taking the dpr into account so as to have crisp lines.
>      this.canvas.setAttribute("width", width * ratio);
>      this.canvas.setAttribute("height", height * ratio);
> -    this.canvas.setAttribute("style", `width:${width}px;height:${height}px`);
> +    this.canvas.setAttribute("style", `box-sizing:border-box;width:${width}px;height:${height}px;border:1px solid green`);

This looks like debugging styles you forgot to remove. I can see the green border around the canvas when enabling the grid inspector.

::: devtools/server/actors/highlighters/css-grid.js:465
(Diff revision 4)
>  
>      // Resize the canvas taking the dpr into account so as to have crisp lines.
>      this.canvas.setAttribute("width", width * ratio);
>      this.canvas.setAttribute("height", height * ratio);
> -    this.canvas.setAttribute("style", `width:${width}px;height:${height}px`);
> -    this.ctx.scale(ratio, ratio);
> +    this.canvas.setAttribute("style", `box-sizing:border-box;width:${width}px;height:${height}px;border:1px solid green`);
> +    // this.ctx.scale(ratio, ratio);

Should this code be removed?

I noticed a regression while testing by the way. Here are STR:
- Open http://labs.jensimmons.com/examples/grid-content-1.html
- Open the inspector and selec the first <main> element you see
- Enable the grid inspector from the rule-view
You should see the grid inspector is too small. It looks like it's zoomed out. It doesn't fit the grid on the page. You can compare on nightly for instance, where it's the right size.
Attachment #8825335 - Flags: review?(pbrosset)
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

https://reviewboard.mozilla.org/r/103506/#review105938

> Doesn't the rulers also need position:fixed like this one?

Rulers have already `position: fixed`, so everything is good there. Unfortunately they'll be still lagging behind since they're based on scroll event – and even if we could probably adapt that now, we'll reintroduce the lag once we're going to have the infinite rulers with better zoom handling; so I didn't bother to change them now.
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

https://reviewboard.mozilla.org/r/103506/#review107436

I see all of my comments have been addressed, and I couldn't find any bugs while testing manually. So this is good to go!
Obviously we still have a lagging problem for scrollable divs/iframes, but it feels so much better for things in the root document!

::: devtools/server/actors/highlighters/auto-refresh.js:195
(Diff revision 5)
> -    return oldQuads !== newQuads;
> +    return areQuadsDifferent(oldQuads, this.currentQuads, getCurrentZoom(this.win));
> +  },
> +
> +  /**
> +   * Update the knowledge we have of the current window's dimensions and return `true`
> +   * if they have change since.

nit: s/change/changed
Attachment #8825335 - Flags: review?(pbrosset) → review+
Alias: devtools/highlighters
Alias: devtools/highlighters
Blocks: 1317102
This patch does not actually disable APZ, does it? As far as I can tell, it just uses position:absolute instead of position:fixed for highlighters that scroll with the main page.
In that case, the commit message should be changed.
Summary: Disable APZ for highlighters → Avoid scrolling latency on highlighters given by APZ
(In reply to Markus Stange [:mstange] from comment #28)

> This patch does not actually disable APZ, does it?

No, it doesn't. It basically tries to avoid when possible to deal with the scrolling itself, for the highlighters. I renamed the bug, and I will change the commit message accordingly. Thanks Markus!
Thanks!
Status: NEW → ASSIGNED
See Also: → 1334523
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

https://reviewboard.mozilla.org/r/103506/#review112562

Apologizes for the unsolicited feedback, but I wanted to make sure we maintain of the code style choices we already have in place in these files.

::: devtools/server/actors/highlighters/auto-refresh.js
(Diff revision 7)
>  
>    this.highlighterEnv = highlighterEnv;
>  
>    this.currentNode = null;
>    this.currentQuads = {};
> -

I think we should keep the new line. We should use new lines to visually separate function bindings, and variable declarations.

::: devtools/server/actors/highlighters/auto-refresh.js:74
(Diff revision 7)
>  
>    this.currentNode = null;
>    this.currentQuads = {};
> -
>    this.update = this.update.bind(this);
> +  this._winDimensions = getWindowDimensions(this.win);

I would put this above the function bindings and perhaps in its own line since it is a private vairable.

::: devtools/server/actors/highlighters/css-grid.js:369
(Diff revision 7)
>     */
>    _update() {
>      setIgnoreLayoutChanges(true);
>  
> +    let root = this.getElement("root");
> +    // hide the root element and force the reflow in order to get the proper window's

s/hide/Hide

::: devtools/server/actors/highlighters/utils/markup.js:534
(Diff revision 7)
>     * @param {String} id The ID of the root element inserted with this API.
>     */
>    scaleRootElement: function (node, id) {
> +    let boundaryWindow = this.highlighterEnv.window;
>      let zoom = getCurrentZoom(node);
> -    let value = "position:absolute;width:100%;height:100%;";
> +    // hide the root element and force the reflow in order to get the proper window's

s/hide/Hide
Attached file Highlighters Stand Alone Test Page (obsolete) — 
This patch needs an extensive manual testing, because it alters the behavior of mostly all the highlighters we have in devtools.

For that reason, and for similar bug we could have in future, I created this stand alone page with some common examples (css positioning, transformation, 3d transformation, iframes…) and generic knowledge about all the current highlighters we have (except the `Rect` highlighter, since it's not used anywhere and it might be removed, see bug 1335691).

For this specific patch, we want to check we haven't introduced any regression, so that all the highlighters have the same behavior with and without the patch applied – the only differences, should be that with the patch applied most of them won't be lag behind during a page's scrolling.

The page attached should helps anyone to do so.
Attachment #8837541 - Flags: feedback?(pbrosset)
Flags: qe-verify?
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07de05d8060c
Avoid scrolling latency on highlighters given by APZ; r=pbro
https://hg.mozilla.org/mozilla-central/rev/07de05d8060c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8837541 [details]
Highlighters Stand Alone Test Page

Hi Matteo, here's some rephrasing feedback in the HTML page:

This highlighter is main and most used among the highlighters, since it's the foundation of elements' inspection. 
==> This highlighter is the most used highlighter because it is used in elements' inspection.

It can be shown just inspect elements in the page
==> It can be shown just by inspecting elements in the page

moving the mouse over the markup view
==> moving the mouse over DOM nodes in the inspector
(the term markup-view is something we use in the team but don't really communicate about to people outside the team, so people might not know what it means)

Open the devtools (Tools > Web Developer > Toggle Tools) 
==> Not sure we need to say this here. You didn't mention this in any other sections, so why here?

an elements's top
==> an element's top

now is possible measuring a portion of the page keeping the mouse's button pressed and release once the interested area is covered. 
==> It is now possible to measure a portion of the page by keeping the mouse button pressed and releasing once the interesting area is covered.

Also, I like the playground area, that's very helpful, but the different examples are just stacked on top of each other, with no relation to the list of highlighters on the right, so it's hard to know which example to use to test a highlighter.
They should either have section titles, or maybe the page should just be 1 column, with description of a highlighter, and then the playground example just below it.
Makes sense?
Attachment #8837541 - Flags: feedback?(pbrosset) → feedback+
(In reply to Patrick Brosset <:pbro> from comment #37)

Hi Patrick, thanks for the feedback! I'll update the page right away.

> Also, I like the playground area, that's very helpful, but the different
> examples are just stacked on top of each other, with no relation to the list
> of highlighters on the right, so it's hard to know which example to use to
> test a highlighter.
> They should either have section titles, or maybe the page should just be 1
> column, with description of a highlighter, and then the playground example
> just below it.
> Makes sense?

The problem I faced with the playground area is that different highlighters could be actually used on the same "example" in the playground area; just imaging for instance the geometry editor vs css transform or box model. My initial thought was to put example under the highlighter description, but then I decided to avoid that – since also I wanted the "framed" part, and I didn't want to reload all the time for all the highlighters. I tried in the highlighter description to highlight (no pun intended!) when it could be used on the playground on the left.

One thing I can definitely do, could be adding a section title – they already have classes that are self-explained – so it would be more clear which example is (e.g. basic css transformation, css positioning, css 3d transformation, grid).
Sounds good?
(In reply to Patrick Brosset <:pbro> from comment #37)

> Open the devtools (Tools > Web Developer > Toggle Tools) 
> ==> Not sure we need to say this here. You didn't mention this in any other
> sections, so why here?

I actually mention that in other sections as well. Basically I split the tools in two category, the one that can be easily activated by inspecting an element (where I've explained how to do so in the "Box Model" section, so I just mention "inspect an element" afterwards); and the one that can be triggered by open the devtools and clicking on some UI icon (e.g. Rulers, Measuring tools).
Flags: needinfo?(pbrosset)
Depends on: 1339800
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #39)
> I actually mention that in other sections as well. Basically I split the
> tools in two category, the one that can be easily activated by inspecting an
> element (where I've explained how to do so in the "Box Model" section, so I
> just mention "inspect an element" afterwards); and the one that can be
> triggered by open the devtools and clicking on some UI icon (e.g. Rulers,
> Measuring tools).
Alright sounds good, thanks.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #38)
> The problem I faced with the playground area is that different highlighters
> could be actually used on the same "example" in the playground area; just
> imaging for instance the geometry editor vs css transform or box model. My
> initial thought was to put example under the highlighter description, but
> then I decided to avoid that – since also I wanted the "framed" part, and I
> didn't want to reload all the time for all the highlighters. I tried in the
> highlighter description to highlight (no pun intended!) when it could be
> used on the playground on the left.
> 
> One thing I can definitely do, could be adding a section title – they
> already have classes that are self-explained – so it would be more clear
> which example is (e.g. basic css transformation, css positioning, css 3d
> transformation, grid).
> Sounds good?
Yes, a title sounds good to me. I just want to make it extra easy for someone testing the highlighters to know where to test.
Flags: needinfo?(pbrosset)
Addressed the comments from Patrick.

Also, currently hosted here: https://zer0.github.io/devtools/highlighters-test-page.html
Attachment #8837541 - Attachment is obsolete: true
Blocks: 1342975
Depends on: 1342929
No longer blocks: 1342975
Depends on: 1342975
We discussed Bug 1342056 today, which was fixed by this one. 
Any plans to uplift Matteo?
Flags: needinfo?(zer0)
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

Approval Request Comment
[Feature/Bug causing the regression]: Since we enabled the APZ
[User impact if declined]: devtools highlighters (box model inspector, css grid inspector, etc) will have a great lag during the scrolling of the page.
[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]: see comment 34
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: it changes the infrastructure of how the highlighters are positioned, so we may got some regression (however, none were found on Nightly from QE).
[Why is the change risky/not risky?]: as above.
[String changes made/needed]: none.
Flags: needinfo?(zer0)
Attachment #8825335 - Flags: approval-mozilla-beta?
(In reply to Julian Descottes [:jdescottes] from comment #45)
> We discussed Bug 1342056 today, which was fixed by this one. 
> Any plans to uplift Matteo?

I was sure we already uplifted this, since was mentioned that we need that patch for fixing other bugs. My bad!
Comment on attachment 8825335 [details]
Bug 1312103 - Avoid scrolling latency on highlighters given by APZ;

This had manual testing in nightly - let's uplift this for beta 52 to fix perf issues in CSS grid.
Attachment #8825335 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Let's make sure this works as intended on Beta 53.
Flags: qe-verify? → qe-verify+
We tested in depth across platforms (Windows 10 64bit, Windows 7 64bit, macOS 10.12.3 and Ubuntu 16.04 64bit using the demopage from comment 41, and we only found one new issue that is not related to this bug. Here are more detalis about our testing: https://public.etherpad-mozilla.org/p/DevTools-1312103-fx53.
I'll go ahead and change the tracking flag for Fx53 as verified, although it has a few new bugs on dependencies (those will be resolved individually).
status-firefox53: fixed → verified
Flags: qe-verify+
OS: Unspecified → All
Hardware: Unspecified → All
Depends on: 1354494
Depends on: 1448995
Product: Firefox → DevTools
No longer depends on: 1448995
Regressions: 1448995
Regressions: 1673561
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: