Closed Bug 1294442 Opened 8 years ago Closed 8 years ago

Popups size calculations need to use an 800px max width when reflowing content to determine dimensions

Categories

(WebExtensions :: Frontend, defect, P1)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox50 verified, firefox51 verified, firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: ecfbugzilla, Assigned: kmag, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: triaged)

Attachments

(3 files)

Minimal extension for testing
(deleted), application/x-xpinstall
Details
Bug 1294442: Part 1 - Allow calculating content size with constraints.
(deleted), text/x-review-board-request
smaug
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.
(deleted), text/x-review-board-request
aswan
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
Attached file Minimal extension for testing (deleted) — 
Steps to reproduce:

1. Go to about:config and make sure xpinstall.signatures.required is set to false (only works on Nightly and Aurora).
2. Install the attached extension.
3. Click the toolbar icon of this extension (generic extension icon) to open its popup.

Expected results:

Text inside the popup is wrapped to multiple lines and is completely visible.

Actual results:

The entire text is displayed on one line. However, the popup size seems to be limited at 800 pixels so everything beyond that is cut off - it isn't even possible to scroll.

Tested in 51.0a1 (2016-08-11) nightly on Mac OS X 10.11. BasePopup._resizeBrowser() function calls docShell.contentViewer.getContentSize() which tries to determine the preferred width of the content - here it appears to be overshooting. As a work-around, one can set max-width:700px on the <body> tag (note: doing that on the document element will currently produce wrong results, see bug 1294440).
I think the problem is that getContentSize reflows the document with no maximum width to determine its preferred dimensions, which seems to override the window dimensions as determined by the browser. We should probably just extend it to allow us to pass a maximum width.
Priority: -- → P3
Whiteboard: triaged
Component: WebExtensions → WebExtensions: Frontend
Summary: Popup contents get cut off if it contains some lengthy text → Popups size calculations need to use an 800px max width when reflowing content to determine dimensions
This may be a good first bug for someone with C++ experience. What I think we need to do is allow passing an optional maximum width to getContentSize (or -1 for no maximum width), and using it instead of `NS_UNCONSTRAINEDSIZE`:

http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/docshell/base/nsIContentViewer.idl#225
http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/layout/base/nsDocumentViewer.cpp#3304

And then passing 800px (probably `800 * window.devicePixelRatio`, actually, to deal with HiDPI scaling) as the maximum width when we call it here:

http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/browser/components/extensions/ext-utils.js#354
Mentor: kmaglione+bmo
Keywords: good-first-bug
Blocks: webext-popups
I have a similar issue, where the popup will not have a vertical scroll bar when the content body overflows over the popup's max height, so the calculation should also take the max height into account.
Assignee: nobody → kmaglione+bmo
Priority: P3 → P1
Comment on attachment 8802348 [details]
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.

https://reviewboard.mozilla.org/r/86762/#review85708

Assuming there are reasonable answers to the questions below, r+

::: browser/components/extensions/ext-utils.js:406
(Diff revision 1)
> -      } catch (e) {
> -        // getContentSize can throw
> -        [width, height] = [400, 400];
> -      }
>  
> -      width = Math.ceil(Math.min(width, 800));
> +      let [width, height] = contentViewer.getContentSizeConstrained(800 * ratio, 600 * ratio);

The old code had this in a try block, from a very quick scan of the first patch, its not obvious to me that this can no longer throw (unless that comment was just out of date?)

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup_resize.js:77
(Diff revision 1)
>    }
>  
> +  yield setSize(1400);
> +
> +  is(panelWindow.innerWidth, 800, "Panel window width");
> +  is(body.clientWidth, 788, "Panel body width");

I take it 12 pixels is the width of the scrollbar?  Is it safe to rely on that being fixed forever?
Attachment #8802348 - Flags: review?(aswan) → review+
Comment on attachment 8802348 [details]
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.

https://reviewboard.mozilla.org/r/86762/#review85708

> The old code had this in a try block, from a very quick scan of the first patch, its not obvious to me that this can no longer throw (unless that comment was just out of date?)

None of the conditions that cause it to throw apply here anymore. The parent docshell check is gone from this version of the function (not that it should have applied anyway), and we never get to this point when the content viewer isn't initialized or doesn't have a document.

> I take it 12 pixels is the width of the scrollbar?  Is it safe to rely on that being fixed forever?

No, it's not safe. I meant to change that to a <= check.
Comment on attachment 8802347 [details]
Bug 1294442: Part 1 - Allow calculating content size with constraints.

https://reviewboard.mozilla.org/r/86760/#review85890

r+, if you switch to use out params.

::: docshell/base/nsIContentViewer.idl:242
(Diff revision 1)
> +  /**
> +   * Returns the preferred width and height of the content, constrained to the
> +   * given maximum values. If either maxWidth or maxHeight is less than zero,
> +   * that dimension is not constrained.
> +   *
> +   * All input and output values are in device pixels, rather than CSS pixels.

Curious, why max values are in device pixels?

::: docshell/base/nsIContentViewer.idl:244
(Diff revision 1)
> +   * given maximum values. If either maxWidth or maxHeight is less than zero,
> +   * that dimension is not constrained.
> +   *
> +   * All input and output values are in device pixels, rather than CSS pixels.
> +   */
> +  void getContentSizeConstrained(in long maxWidth, in long maxHeight,

For consistency with getContentSize, this should also return out long width, out long height, not some array where one needs to guess which item means what value.

::: layout/base/nsDocumentViewer.cpp:3437
(Diff revision 1)
> +  return GetContentSizeInternal(aWidth, aHeight, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
> +}
> +
> +NS_IMETHODIMP
> +nsDocumentViewer::GetContentSizeConstrained(int32_t aMaxWidth, int32_t aMaxHeight,
> +                                            uint32_t* aCount, int32_t** aDimensions)

So, make the last two params
int32_t* aWidth, int32_t* aHeight

::: layout/base/nsDocumentViewer.cpp:3456
(Diff revision 1)
> +
> +  int32_t width = 0, height = 0;
> +  nsresult rv = GetContentSizeInternal(&width, &height, maxWidth, maxHeight);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  *aCount = 2;

...then you don't need this stuff...

::: layout/base/nsDocumentViewer.cpp:3460
(Diff revision 1)
> +
> +  *aCount = 2;
> +  *aDimensions = static_cast<int32_t*>(
> +    moz_xmalloc(*aCount * sizeof **aDimensions));
> +
> +  (*aDimensions)[0] = width;

and can assign the out values here, and keep the method consistent with the old GetContentSize.

(And in case someone uses this in C++, returning array is quite error prone. Need to remember to free the return value and so)
Attachment #8802347 - Flags: review?(bugs) → review+
Comment on attachment 8802347 [details]
Bug 1294442: Part 1 - Allow calculating content size with constraints.

https://reviewboard.mozilla.org/r/86760/#review85890

> Curious, why max values are in device pixels?

Just for consistency with the output params. I could switch them both, but that would require either being inconsistent with `getContentSize`, or changing other callers of that too.

> For consistency with getContentSize, this should also return out long width, out long height, not some array where one needs to guess which item means what value.

I was just trying to avoid the clunkiness of using out params in JS, since there aren't any native callers. But I don't have that strong an opinion of it, so I'll change to out params.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d66bd1c7489b5a792aa0fca5ed9a0c41e4e6b3b
Bug 1294442: Part 1 - Allow calculating content size with constraints. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe922c12cb08ab43cedb934459c554a4a0a64d6
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum. r=aswan
Comment on attachment 8802348 [details]
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.

Approval Request Comment
[Feature/regressing bug #]: Bug 1215025
[User impact if declined]: This bug causes layout issues in browserAction and pageAction popups, where over-sized content becomes entirely hidden off-screen, rather than being accessible using scrollbars. This has been reported as an issue in several different add-ons recently, and is particularly likely to come up as more add-ons are ported from Chrome.
[Describe test coverage new/current, TreeHerder]: The resizing code that this change affects is thoroughly covered by existing tests, and this patch adds new tests for the changed behavior.
[Risks and why]: Low. The size restrictions that this patch deals with are not new. The change simply moves their implementation directly into the native layout code that calculates the preferred size, so that the calculation doesn't leave internal and external size representations in an inconsistent state.
[String/UUID change made/needed]: None.
Attachment #8802348 - Flags: approval-mozilla-beta?
Attachment #8802348 - Flags: approval-mozilla-aurora?
Comment on attachment 8802347 [details]
Bug 1294442: Part 1 - Allow calculating content size with constraints.

Approval Request Comment: See comment 13.
Attachment #8802347 - Flags: approval-mozilla-beta?
Attachment #8802347 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f6411c994f5fd91094834f3a1c7997f87c5825
Bug 1294442: Follow-up: Fix additional windows-only test failures. r=bustage
Hello Wladimir, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(trev.moz)
Comment on attachment 8802348 [details]
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.

Let's stabilize this on Aurora51 for a few days and take it in Beta50 before I gtb Beta10 on Monday. Also hoping to get verification from bug opener in the mean time.
Attachment #8802348 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8802347 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Vasilica, can you also confirm that this resolves the issue that you reported?

Thanks.
Flags: needinfo?(vasilica.mihasca)
For the record, I folded the two followups into patch 2 before uplifting.
(In reply to Kris Maglione [:kmag] from comment #20)
> Vasilica, can you also confirm that this resolves the issue that you
> reported?

I confirm that the scenario reported in Bug 1310947 and the issue mentioned in Description are no longer reproducible on Firefox 52.0a1 (2016-10-21) under Windows 10 64-bit and Ubuntu 16.04 32-bit: http://screencast.com/t/9FgfOGOl , http://screencast.com/t/kC20UKFT9ah

Based on this testing I am marking this bug as Verified!

Thanks Kris!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: needinfo?(vasilica.mihasca)
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0fa7e0336c5aab0ad3a50bbb7c7582cfc790707
https://hg.mozilla.org/releases/mozilla-aurora/rev/96ca0ecdcfa7b14cab40e8c7a0a4d97441652a79

These should also work fine on beta, now. There was a timing issue in the added tests due to CSS animations on Windows, but this version retries until the popup reaches the expected size.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8802348 [details]
Bug 1294442: Part 2 - Fix layout issues when popup's preferred size is larger than maximum.

Fix has stabilized on Nightly52 and Aurora51 and was verified. Beta50+
Attachment #8802348 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8802347 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Part 2 is hitting conflicts uplifting to beta, could we get a rebased patch?
Flags: needinfo?(kmaglione+bmo)
(In reply to Ritu Kothari (:ritu) from comment #18)
> Hello Wladimir, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

Yes, it is fixed. I reverted the hack setting a fixed document width in Easy Passwords and it still won't overflow.
Flags: needinfo?(trev.moz)
Confirm that this bug is also fixed on Firefox 51.0a2 (2016-10-27) and Firefox 50.0b11 (20161027110534) under Windows 10 64-bit, Ubuntu 12.04 64-bit and Mac OS X 10.12.1. The webextension pop-up is correctly displayed.
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: