Closed Bug 1324499 Opened 7 years ago Closed 7 years ago

WebExtension browser action popup scrolls to the top on changing an element's style

Categories

(WebExtensions :: Frontend, defect, P2)

50 Branch
defect

Tracking

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: justin.mopp, Assigned: angelsl)

References

Details

(Keywords: regression, testcase, Whiteboard: triaged)

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161209094039

Steps to reproduce:

I unzipped the attached web extension and loaded the manifest as a temporary add-on from about:debugging. I then clicked on the extension's icon, scrolled the popup down to around the middle, and clicked on a list item.


Actual results:

The list item's style changed - as expected, but the popup instantly scrolled to the top.


Expected results:

The popup should have kept its scroll position. I tested this in Firefox 49 and it worked as expected.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Ever confirmed: true
filing another bug, someone from layout team planning on working on it.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
Was there a follow up bug on this? Or was Kris going to do that.
Flags: needinfo?(sescalante)
Whiteboard: triaged
I'm experiencing the same issue. I made a few tests and it seems any change in the DOM triggers the scroll to the top (not only changes that modify the height of the popup).

I found this issue in the process of converting my Chrome extension into a WebExtension. The problem doesn't occur on Chrome.
uMatrix WebExtension suffers from this bug as well: hovering over the matrix cells will always reset the scroll position to the top. So if you have a long list of domains and want to set a rule for a row which is visible only if you scroll down, the matrix will jump back to the top.

This bug is not reproduced if I move uMatrix icon to the overflow menu(right click on uMatrix icon - Pin to Overflow Menu). However, there is no horizontal scrollbar and the width of the overflow menu is too small so the most part of the uMatrix dashboard is not visible. I don't know if that's intended or not.
Rephrasing comment 5 a bit: this bug renders uMatrix/webex unusable, and since uMatrix non-webex was rendered unusable by recent nightly updates, there isn't a working version anymore. Plenty of people are waiting for this to be fixed.
It's a big issue on our side too. The only thing I can do is to reduce the height of the popup the maximum I can but it's most of the time not really possible. It's really painful for the users.

Please let me know if I can help with more details about the issue and the steps to reproduce.
It appears the issue is that the first reflow in nsDocumentViewer::GetContentSizeInternal [1], which reflows the content with unlimited height (to get its full height) naturally sets the scroll top to zero, and the original scroll top is lost.

After the second reflow with maximum height specified, the scroll top stays at zero.

[1]: https://dxr.mozilla.org/mozilla-central/rev/5ca5691372cb432ec1fa4693ca608a30858226de/layout/base/nsDocumentViewer.cpp#3571
---

Thread 1 "firefox" hit Hardware watchpoint 13: *((int *) 0x7f4a122932ec)

Old value = -53280
New value = 0
mozilla::ScrollFrameHelper::ScrollToImpl (this=this@entry=0x7f4a12293708, aPt=..., aRange=..., aOrigin=0x7f4a1b3796e0, aOrigin@entry=0x0)
    at layout/generic/nsGfxScrollFrame.cpp:2821
2821	    nsLayoutUtils::CanScrollOriginClobberApz(mLastScrollOrigin) &&
(gdb) bt
#0  0x00007f4a221cf8ad in mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsIAtom*) (this=this@entry=0x7f4a12293708, aPt=..., aRange=..., aOrigin=0x7f4a1b3796e0, 
    aOrigin@entry=0x0) at layout/generic/nsGfxScrollFrame.cpp:2821
#1  0x00007f4a221d2921 in mozilla::ScrollFrameHelper::ReflowFinished() (this=0x7f4a12293708) at layout/generic/nsGfxScrollFrame.cpp:5335
#2  0x00007f4a220b65b7 in mozilla::PresShell::HandlePostedReflowCallbacks(bool) (this=this@entry=0x7f4a12290000, aInterruptible=aInterruptible@entry=true)
    at layout/base/PresShell.cpp:4023
#3  0x00007f4a220b667b in mozilla::PresShell::DidDoReflow(bool) (this=this@entry=0x7f4a12290000, aInterruptible=aInterruptible@entry=true)
    at layout/base/PresShell.cpp:9183
#4  0x00007f4a220b6a20 in mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, int, int) (this=0x7f4a12290000, aWidth=9060, aHeight=1073741823, aOldWidth=<optimized out>, aOldHeight=<optimized out>) at layout/base/PresShell.cpp:1987
#5  0x00007f4a220d0763 in nsDocumentViewer::GetContentSizeInternal(int*, int*, int, int) (this=this@entry=0x7f49f3c5f8b0, aWidth=aWidth@entry=0x7fffffff8bb8, aHeight=aHeight@entry=0x7fffffff8bd0, aMaxWidth=<optimized out>, aMaxHeight=36000) at layout/base/nsDocumentViewer.cpp:3571
#6  0x00007f4a220d0a5c in nsDocumentViewer::GetContentSizeConstrained(int, int, int*, int*) (this=0x7f49f3c5f8b0, aMaxWidth=800, aMaxHeight=600, aWidth=0x7fffffff8bb8, aHeight=0x7fffffff8bd0)
    at layout/base/nsDocumentViewer.cpp:3633
Attachment #8899278 - Flags: review?(tnikkel)
Attachment #8899278 - Flags: review?(dholbert)
Attachment #8899278 - Flags: review?(dbaron)
Attachment #8899278 - Flags: review?(xidorn+moz)
Attachment #8899278 - Flags: review?(bugs)
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review175656

You don't need to ask so many people to review. One reviewer should be enough.

The change in this patch isn't in my familiar area, and it seems to be touching some core layout code. Although I can try to review, I would probably prefer asking dbaron or dholbert to have a look.

::: layout/base/PresShell.cpp:1899
(Diff revision 1)
>  {
>    static_cast<PresShell*>(aPresShell)->FireResizeEvent();
>  }
>  
>  nsresult
> -PresShell::ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight)
> +PresShell::ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint)

This line seems to be too long. Please break it into multiple lines.

::: layout/base/PresShell.cpp:1915
(Diff revision 1)
>      // ResizeReflowIgnoreOverride if it changed.
>      mMobileViewportManager->RequestReflow();
>      return NS_OK;
>    }
>  
> -  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight);
> +  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight, aHeightIsConstraint);

So does this line.

::: layout/base/PresShell.cpp:1919
(Diff revision 1)
>  
> -  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight);
> +  return ResizeReflowIgnoreOverride(aWidth, aHeight, aOldWidth, aOldHeight, aHeightIsConstraint);
>  }
>  
>  nsresult
> -PresShell::ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight)
> +PresShell::ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint)

and this.

::: layout/base/nsIPresShell.h:369
(Diff revision 1)
>  
>    /**
>     * Reflow the frame model into a new width and height.  The
>     * coordinates for aWidth and aHeight must be in standard nscoord's.
>     */
> -  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0) = 0;
> +  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0, bool aHeightIsConstraint = false) = 0;

and this.

::: layout/base/nsIPresShell.h:374
(Diff revision 1)
> -  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0) = 0;
> +  virtual nsresult ResizeReflow(nscoord aWidth, nscoord aHeight, nscoord aOldWidth = 0, nscoord aOldHeight = 0, bool aHeightIsConstraint = false) = 0;
>    /**
>     * Do the same thing as ResizeReflow but even if ResizeReflowOverride was
>     * called previously.
>     */
> -  virtual nsresult ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight) = 0;
> +  virtual nsresult ResizeReflowIgnoreOverride(nscoord aWidth, nscoord aHeight, nscoord aOldWidth, nscoord aOldHeight, bool aHeightIsConstraint = false) = 0;

and this.
Attachment #8899278 - Flags: review?(xidorn+moz)
Attachment #8899278 - Flags: review?(tnikkel)
Attachment #8899278 - Flags: review?(bugs)
Could you include an explanation of what the patch is trying to accomplish?
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> Could you include an explanation of what the patch is trying to accomplish?

You mean elaborate the commit message?
Yes.  Now that I had a little more time to look at the bug, I see that the first 3 lines of comment 9 contain a brief description of the underlying problem, but they don't explain what approach you took to fix it.
A few other comments though:

 - at a low level, I think the aHeightIsContstraint should, throughout the patch, be aHeightIs*Un*constrain*ed* (two changes).  But I might be misunderstanding

 - at a slightly higher level, it's not clear to me why you're not trying to fix the bug at a slightly higher level (in nsDocumentViewer::GetContentSizeInternal), by making the code that decides to reflow at an unconstrained height save and restore the scroll position.  (You may or may not want to use the nsIStatefulFrame implementation on the scroll frame to do that.)
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review175724

Please re-request review after addressing comment 12 / comment 14 / comment 15.
Attachment #8899278 - Flags: review?(dbaron)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #15)
>  - at a low level, I think the aHeightIsContstraint should, throughout the
> patch, be aHeightIs*Un*constrain*ed* (two changes).  But I might be
> misunderstanding

I renamed the parameter to aHeightIsLimit: when true, aHeight is a limit to height
(i.e. the final height can be smaller) instead of a fixed dimension.

>  - at a slightly higher level, it's not clear to me why you're not trying to
> fix the bug at a slightly higher level (in
> nsDocumentViewer::GetContentSizeInternal), by making the code that decides
> to reflow at an unconstrained height save and restore the scroll position. 
> (You may or may not want to use the nsIStatefulFrame implementation on the
> scroll frame to do that.)

I thought it'd be better (?) if we simply don't lose the scroll position.

But I'll push another change with that fix; you can accept the one you prefer.
Attachment #8899278 - Flags: review?(dholbert)
Attachment #8899278 - Flags: review?(dbaron)
I realised that the current way of reflowing to determine size causes a lot of spurious JS scroll & resize events.

Try out scrollbug_with_events.xpi and look in the addon's console.

I guess another bug should be filed about this? The fix might be to only fire resize/scroll events if the size/scroll position actually changed, which the code currently doesn't check for.
(In reply to grante.francois from comment #7)
> It's a big issue on our side too. The only thing I can do is to reduce the
> height of the popup the maximum I can but it's most of the time not really
> possible. It's really painful for the users.

FWIW as a workaround, you can wrap your whole popup contents in a div limited
to max-height 600px. The bug only affects the scroll of the root HTML frame,
so if your root HTML frame doesn't scroll, then the bug is avoided.
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

https://reviewboard.mozilla.org/r/170734/#review176042

r=dbaron with the following change

::: layout/base/nsDocumentViewer.cpp:3595
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      shellArea = presContext->GetVisibleArea();
> +
> +    // the first reflow reset our scroll, now set it back
> +    if (frameState) {

For safety, please also add a test here that
presShell->GetRootScrollFrameAsScrollable() == scrollFrame (so we don't enter this codepath if the frame pointer is different).
Attachment #8899459 - Flags: review?(dbaron) → review+
Comment on attachment 8899278 [details]
Bug 1324499 - Constrain height before calling DidDoReflow callbacks

https://reviewboard.mozilla.org/r/170520/#review176094
Attachment #8899278 - Flags: review?(dbaron)
Attachment #8899278 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca71f04513ab

Can someone add checkin-needed?
Assignee: nobody → angelsl
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/76c7aa772dc4
Save and restore scroll position after reflow in GetContentSizeInternal r=dbaron
https://hg.mozilla.org/mozilla-central/rev/76c7aa772dc4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1294442
[User impact if declined]: Webexts with popups that are larger than 600px content height may become unusable (e.g. uMatrix)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Install the webext in attachment 8819956 [details], open the popup, scroll down and click on a list item
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Code changes a method currently only used by webext popups
[String changes made/needed]: None
Attachment #8899459 - Flags: approval-mozilla-beta?
Flags: needinfo?(sescalante)
Flags: needinfo?(kmaglione+bmo)
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+
I can still reproduce this bug in Nightly 23-08-17. STR:
Install uMatrix beta https://addons.mozilla.org/en-US/firefox/addon/umatrix/versions/beta
Go to http://www.dailymail.co.uk/ushome/index.html
Open uMatrix dashboard, try to scroll down

AR: hovering over the matrix cells resets the scroll position to the top
This patch should be in the next Nightly.
Nightly 24-08-17: I can no longer reproduce the bug using STR from comment 34. Great job, Angelsl! Thank you very much! \o/
Thanks for the patch, angelsl! Welcome onboard. :) Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#August_2017

I look forward to seeing you around!
Comment on attachment 8899459 [details]
Bug 1324499: Save and restore scroll position after reflow in GetContentSizeInternal

Fix a WebExtension related issue and was verified. Beta56+.
Attachment #8899459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've reproduced this issue following the STR from comment 0, using an old Nightly build.

This is verified fixed on latest Nightly 57.0a1 (2017-09-13) and 56 Beta 11 (20170911193316) across platforms: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.