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)
WebExtensions
Frontend
Tracking
(firefox50 verified, firefox51 verified, firefox52 verified)
VERIFIED
FIXED
mozilla52
People
(Reporter: jwkbugzilla, Assigned: kmag, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: triaged)
Attachments
(3 files)
787 bytes,
application/x-xpinstall
|
Details | |
58 bytes,
text/x-review-board-request
|
smaug
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
aswan
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
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).
Comment 1•8 years ago
|
||
Duplicate from https://bugzilla.mozilla.org/show_bug.cgi?id=1287305 ?
Assignee | ||
Comment 2•8 years ago
|
||
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.
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: triaged
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Blocks: webext-popups
Comment 4•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → kmaglione+bmo
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-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
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+
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
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 10•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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?
Assignee | ||
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/861ee8c0e9d03ad516faed3f6f94420198300613
Bug 1294442: Follow-up: Fix test failures. r=bustage
status-firefox50:
--- → affected
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f6411c994f5fd91094834f3a1c7997f87c5825
Bug 1294442: Follow-up: Fix additional windows-only test failures. r=bustage
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d66bd1c7489
https://hg.mozilla.org/mozilla-central/rev/cbe922c12cb0
https://hg.mozilla.org/mozilla-central/rev/861ee8c0e9d0
https://hg.mozilla.org/mozilla-central/rev/73f6411c994f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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+
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 22•8 years ago
|
||
bugherder uplift |
I had to back these out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3924351&repo=mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/d61548e284ca
Flags: needinfo?(kmaglione+bmo)
Comment 24•8 years ago
|
||
(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!
Assignee | ||
Comment 25•8 years ago
|
||
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)
Assignee | ||
Comment 28•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/e007b7fbc4f24c264d96149802d6ee74b9754521
https://hg.mozilla.org/releases/mozilla-beta/rev/0c671ce41082a2bba72b4e2340c391f9a5d83b6b
Flags: needinfo?(kmaglione+bmo)
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1857476&repo=mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/38cfded17052
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 30•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/2afe0634177930d041f324bf3bccefd3c77afb7e
https://hg.mozilla.org/releases/mozilla-beta/rev/7381df3610080d63beb74320fcf055267a231d94
Flags: needinfo?(kmaglione+bmo)
Out again for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1865431&repo=mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/565744df4173
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 32•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/ca38705921e6193da2d8c2a55480287fffe602d6
https://hg.mozilla.org/releases/mozilla-beta/rev/49d808e29f639134d1f1510084a962f7924d9977
Flags: needinfo?(kmaglione+bmo)
Reporter | ||
Comment 33•8 years ago
|
||
(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)
Comment 34•8 years ago
|
||
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.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•