Closed Bug 1047586 Opened 5 years ago Closed 5 years ago

Unable to interact with in-content preferences after changing minimum font size to a very large value

Categories

(Firefox :: Preferences, defect, P2, major)

defect
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: alice0775, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [strings])

Attachments

(2 files, 1 obsolete file)

Steps To Reproduce:
1. Open preferences
2. Select Content
3. Click Advanced...
4. Change Minimum font size to big such as 36

Actual Results:
The in-content preferences is completely broken.
No Scrollbar.
Unable to interact.
Attached image bug screenshot
[Tracking Requested - why for this release]:
Keywords: regression
It looks like there are parts of the CSS that have fixed sizes, and the text is overflowing these fixed sizes. If we want to fix this the right way we'll need to replace the fixed sized areas with areas that can grow to the size of their text.
Points: --- → 5
in-content prefs is not shipping in 34. Dropping tracking.
Mentor: jaws, ntim007
Whiteboard: [good first bug]
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
Whiteboard: [good first bug] → [good first bug][lang=css]
This doesn't look like a [good first bug] to me, as it's unclear what the values should be changed to, as well as what values we should change and what can remain. Should we have a max font-size that we support? Or should we not allow the font-size changes in the preferences to apply to the preferences (this would fix 1008169)?
Whiteboard: [good first bug][lang=css] → [lang=css]
I think the right fix probably involves the minimum font size not affecting about:preferences - I don't think we should change what values are available in the list (if only because if people changed this previously, I don't know how the pref pane will react).

I'll look into this.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Flags: qe-verify-
Flags: in-testsuite?
Whiteboard: [lang=css]
So as I might have guessed, this boils down to nsLayoutUtils and/or layout/style/nsRuleNode asking the prescontext if it's chrome, which boils down to whether the docshell is a chrome docshell. Because we're loading in a tab, that's not the case.

Boris, we're going to run into this more, even for HTML, where it doesn't strictly make sense to apply some of these content-related prefs to the chrome bits (where we didn't before because they were in a window).

Equally, we probably don't want to change the docshell type here (if only because it's asking for security issues and is complex implementation-wise).

Is it a dumb idea to build a whitelist for this kind of thing? I kind of don't want to exclude any and all chrome: or even about: pages (thinking particularly of about:blank but also of things like about:reader for reader mode (which should probably honor colour and font size prefs), etc.).
Flags: needinfo?(bzbarsky)
> Because we're loading in a tab, that's not the case.

This. This is what's broken about our current "loading in a tab" stuff when we load privileged things in it.  dveditz and I have been asking for years (going on a decade now) for the Firefox UI to stop loading privileged stuff in content-type docshells, and we really need to stop doing that and adding hacks in Gecko to support it and whatnot.

> Equally, we probably don't want to change the docshell type here 

Docshell types are immutable.  What you _want_ is to just use a different docshell.  For example, when opening preferences, create a chrome-type docshell, load the stuff in it, disable the url bar while that tab is focused (or make navigation via the URL bar close that tab and open a new one for the navigation).

Similarly, when the user types about:preferences in the URL bar, open it in a new tab.

Ideally, we'd make loads of system-principal stuff in content docshells simply fail.  We've wanted to do that for security reasons for a long time...  We'd need to figure out extension compat for this, of course.

We could add more complexity of some sort in Gecko to deal with this, but it really seems like a better idea to fix this on the Firefox end, since Firefox is what knows which things being loaded in tabs are actually "UI" as opposed to just something someone loaded in a tab.
Flags: needinfo?(bzbarsky)
One other note.  e10s _already_ does some sort of docshell switcheroo here, right?  I mean, about:preferences runs in the chrome process, while normal web pages run in the content process...  How complicated would it be to leverage that in the non-e10s case?
> How complicated would it be to leverage that in the non-e10s case?

... and use a chrome-type docshell in the e10s case, I guess.
(In reply to Boris Zbarsky [:bz] from comment #11)
> > How complicated would it be to leverage that in the non-e10s case?
> 
> ... and use a chrome-type docshell in the e10s case, I guess.

It'll probably break add-ons (the e10s switch broke add-ons, it just wasn't a massive deal in the sense that it "only" happened on Nightly). We use a different tabbrowser binding in the e10s case, I'm not sure how easy it would be to do this without requiring a new binding.

(In reply to Boris Zbarsky [:bz] from comment #9)
> > Because we're loading in a tab, that's not the case.
> 
> This. This is what's broken about our current "loading in a tab" stuff when
> we load privileged things in it.  dveditz and I have been asking for years
> (going on a decade now) for the Firefox UI to stop loading privileged stuff
> in content-type docshells, and we really need to stop doing that and adding
> hacks in Gecko to support it and whatnot.

While I get that it is problematic from a core/docshell support perspective, design-wise, we're moving away from separate dialogs, so this just doesn't seem likely to go away.

We could theoretically rewrite the prefs window to be unprivileged and just interface with a content script that is privileged instead, but it'd be silly amounts of work - and it wouldn't fix this bug.

> > Equally, we probably don't want to change the docshell type here 
> 
> Docshell types are immutable.  What you _want_ is to just use a different
> docshell.  For example, when opening preferences, create a chrome-type
> docshell, load the stuff in it, disable the url bar while that tab is
> focused (or make navigation via the URL bar close that tab and open a new
> one for the navigation).
> 
> Similarly, when the user types about:preferences in the URL bar, open it in
> a new tab.

While I completely understand why this would be desirable from a security perspective, I can already hear the UX/design folks screaming. :-)

OOI, when we switch in the same way that e10s does, would this still be required?

> Ideally, we'd make loads of system-principal stuff in content docshells
> simply fail.  We've wanted to do that for security reasons for a long
> time...  We'd need to figure out extension compat for this, of course.
> 
> We could add more complexity of some sort in Gecko to deal with this, but it
> really seems like a better idea to fix this on the Firefox end, since
> Firefox is what knows which things being loaded in tabs are actually "UI" as
> opposed to just something someone loaded in a tab.

The separation isn't quite that harsh though, in that platform code has plenty of ideas about e.g. which about: pages are privileged and which aren't...
> so this just doesn't seem likely to go away

I don't think you understood what I was saying.  I'm not saying you shouldn't load stuff in what looks like tabs.  I'm saying you need to stop using content-type docshells for parts of the UI.

> We could theoretically rewrite the prefs window to be unprivileged

That's not at all what I suggested, right?

> I can already hear the UX/design folks screaming. :-)

Why, exactly?  Right now, going to the "preferences" menu item opens a new tab.  Do you really think so many normal people type "about:preferences" in the url bar?

> OOI, when we switch in the same way that e10s does, would this still be required?

Probably not, no.

> in that platform code has plenty of ideas about e.g. which about: pages are privileged
> and which aren't

That's true.  But we're not talking about privileged vs not.  We're talking about "part of the browser UI" vs not, which is not the same thing: you can have unprivileged things that are part of the browser UI, and privileged ones that are not.
(In reply to Boris Zbarsky [:bz] from comment #13)
> > so this just doesn't seem likely to go away
> 
> I don't think you understood what I was saying.

Maybe. I think what confused me was that you initially pointed out that loading privileged stuff in content-type docshells was bad.

>  I'm not saying you
> shouldn't load stuff in what looks like tabs.  I'm saying you need to stop
> using content-type docshells for parts of the UI.

Yes, it's just that I think that might be quite difficult to do. I would prefer a simpler solution given timescales and the size of the problem here (ie how many people are actually going to set their minimum font-size to 72).

> > We could theoretically rewrite the prefs window to be unprivileged
> 
> That's not at all what I suggested, right?

No, but it seemed that the root of your ire was directed at "privileged stuff in content-type docshells". While changing the docshell type is one way to address that, it seemed complex to me, so I wanted to clarify if attacking the other side of that issue was possible. To me it doesn't seem like that's the case, and it sounds like you don't think it's a good idea either.

> > I can already hear the UX/design folks screaming. :-)
> 
> Why, exactly?  Right now, going to the "preferences" menu item opens a new
> tab.  Do you really think so many normal people type "about:preferences" in
> the url bar?

I agree there are probably few people who do that. I'm not sure about people who then put other things in the URL bar that aren't about:preferences, while about:preferences is open, which AIU your comment would have the same effect (ie open a new tab, rather than navigating the browser/docshell for that tab).


I can look at the docshell-type solution. I just wonder if it might not be simpler to make the prefwindow cope with different font sizes...
> loading privileged stuff in content-type docshells was bad.

Yes, it generally is.  We should stop doing that.  The most common reason we do it is because we load part of the browser UI in a tab.

> I would prefer a simpler solution

Yes, that's what we've been saying for 10+ years as we accrete "simpler" hacks...

> To me it doesn't seem like that's the case, and it sounds like you don't think it's a
> good idea either.

Correct.

> which AIU your comment would have the same effect

What I'd recommend it do is open a new tab, close the about:preferences tab...  Then you can't hit the back button to go back to about:preferences, of course, but that might not be fatal...

> I just wonder if it might not be simpler to make the prefwindow cope with different font
> sizes.

That's obviously your call.  ;)
(In reply to :Gijs Kruitbosch from comment #14)
> I just wonder if it might not be
> simpler to make the prefwindow cope with different font sizes...

There is certainly something to be said about supporting users who have gone out of their way to use large fonts. Forcing a fixed font-size on the preferences could make the preferences unusable to someone who requires large fonts.

With that being said, we should look at how well our support for large fonts is in the tabbar. If the browser doesn't handle large fonts in the tabbar well, then I think we can lower the priority of this bug.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> (In reply to :Gijs Kruitbosch from comment #14)
> > I just wonder if it might not be
> > simpler to make the prefwindow cope with different font sizes...
> 
> There is certainly something to be said about supporting users who have gone
> out of their way to use large fonts. Forcing a fixed font-size on the
> preferences could make the preferences unusable to someone who requires
> large fonts.
> 
> With that being said, we should look at how well our support for large fonts
> is in the tabbar. If the browser doesn't handle large fonts in the tabbar
> well, then I think we can lower the priority of this bug.

The tabbar isn't affected by the minimum font size in gecko though - it's in a chrome docshell. All it has to care about is OS font sizes, and it deals with moderately large ones (200% on Windows, larger fonts configured on Linux) reasonably well. It might look non-optimal, but it doesn't become unusable like it does here.

The main issue is that there's a lot of heights/widths defined in px, which therefore don't scale very well when you start changing font sizes. There's also XUL weirdness going on in terms of scrollbars and so on (see comment #0).

I'm not sure how easy it is to fix the layout though, e.g. when I looked, media queries like max-width seem to ignore the minimum font-size (and assume the document's default font size is still whatever it would otherwise be). Simple test:

http://jsbin.com/coricuhemu/1/edit

Doesn't seem to be affected by the minimum font size in terms of when the background switches from green to red...

Then again, how many users set their font size to be >24px? (which seems to work pretty OK-ish for me, even if it doesn't look great)

Finally, note that other in-content pages like about:config have similar issues (it's almost unusable at 40px).

I like non-trivial problems like this, but I'm not sure how big a problem this is in practice. It probably shouldn't be P1.
Severity: critical → major
Priority: P1 → P2
Summary: Unable to interact with In-content preferences after changing Font size → Unable to interact with in-content preferences after changing minimum font size to a very large value
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Gijs and I discussed this bug and decided on the following solution:

1. File a new bug that will track making our in-content pages (about:preferences, about:config, etc) work better with larger font sizes (converting sizes from px to em).
2. For this bug, we will display a warning in the Advanced dialog when font-sizes larger than 24px are chosen. This dialog is not instant-apply, so users will have the opportunity to undo the change before hitting OK.
Whiteboard: [strings]
Attached patch warn about large fonts, (obsolete) — Splinter Review
Attachment #8564351 - Flags: review?(jaws)
Comment on attachment 8564351 [details] [diff] [review]
warn about large fonts,

Review of attachment 8564351 [details] [diff] [review]:
-----------------------------------------------------------------

I've tried to make the strings a little easier to read.

The alternate approach to this bug that I was thinking is that we would just show the warning text within the dialog when any of the font sizes are changed to a higher-than-24px value. That may be simpler code-wise, but also doesn't require that the person explicitly override this.

::: browser/components/preferences/fonts.js
@@ +136,5 @@
> +    let warningMessage = strings.getString("veryLargeMinimumFontWarning");
> +    let {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});
> +    let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING |
> +                Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING;
> +    let shouldReduce = Services.prompt.confirmEx(window, title, warningMessage, flags, reduceLabel, confirmLabel, "", "", {});

It would be nice to also include a Cancel button that allows neither option to be chosen.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +13,5 @@
>  
>  labelDefaultFont=Default (%S)
>  
> +veryLargeMinimumFontTitle=Large minimum font size
> +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?

veryLargeMinimumFontWarning=Some of the selected font sizes are known to cause issues with &brandShortName;. Font sizes 24px or smaller are recommended.

@@ +14,5 @@
>  labelDefaultFont=Default (%S)
>  
> +veryLargeMinimumFontTitle=Large minimum font size
> +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> +acceptVeryLargeMinimumFont=Set minimum size

acceptVeryLargeMinimumFont=I understand, please keep the large font sizes

@@ +15,5 @@
>  
> +veryLargeMinimumFontTitle=Large minimum font size
> +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> +acceptVeryLargeMinimumFont=Set minimum size
> +reduceMinimumFont=Reduce these

reduceMinimumFont=Lower the affected font sizes to 24 pixels
Attachment #8564351 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Comment on attachment 8564351 [details] [diff] [review]
> warn about large fonts,
> 
> Review of attachment 8564351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've tried to make the strings a little easier to read.
> 
> The alternate approach to this bug that I was thinking is that we would just
> show the warning text within the dialog when any of the font sizes are
> changed to a higher-than-24px value. That may be simpler code-wise, but also
> doesn't require that the person explicitly override this.
> 
> ::: browser/components/preferences/fonts.js
> @@ +136,5 @@
> > +    let warningMessage = strings.getString("veryLargeMinimumFontWarning");
> > +    let {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});
> > +    let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING |
> > +                Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING;
> > +    let shouldReduce = Services.prompt.confirmEx(window, title, warningMessage, flags, reduceLabel, confirmLabel, "", "", {});
> 
> It would be nice to also include a Cancel button that allows neither option
> to be chosen.
> 
> ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
> @@ +13,5 @@
> >  
> >  labelDefaultFont=Default (%S)
> >  
> > +veryLargeMinimumFontTitle=Large minimum font size
> > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> 
> veryLargeMinimumFontWarning=Some of the selected font sizes are known to
> cause issues with &brandShortName;. Font sizes 24px or smaller are
> recommended.
> 
> @@ +14,5 @@
> >  labelDefaultFont=Default (%S)
> >  
> > +veryLargeMinimumFontTitle=Large minimum font size
> > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> > +acceptVeryLargeMinimumFont=Set minimum size
> 
> acceptVeryLargeMinimumFont=I understand, please keep the large font sizes
> 
> @@ +15,5 @@
> >  
> > +veryLargeMinimumFontTitle=Large minimum font size
> > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> > +acceptVeryLargeMinimumFont=Set minimum size
> > +reduceMinimumFont=Reduce these
> 
> reduceMinimumFont=Lower the affected font sizes to 24 pixels

While I like your suggestions, aren't these a bit long for button text?
(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > Comment on attachment 8564351 [details] [diff] [review]
> > warn about large fonts,
> > 
> > Review of attachment 8564351 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I've tried to make the strings a little easier to read.
> > 
> > The alternate approach to this bug that I was thinking is that we would just
> > show the warning text within the dialog when any of the font sizes are
> > changed to a higher-than-24px value. That may be simpler code-wise, but also
> > doesn't require that the person explicitly override this.
> > 
> > ::: browser/components/preferences/fonts.js
> > @@ +136,5 @@
> > > +    let warningMessage = strings.getString("veryLargeMinimumFontWarning");
> > > +    let {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});
> > > +    let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING |
> > > +                Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING;
> > > +    let shouldReduce = Services.prompt.confirmEx(window, title, warningMessage, flags, reduceLabel, confirmLabel, "", "", {});
> > 
> > It would be nice to also include a Cancel button that allows neither option
> > to be chosen.
> > 
> > ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
> > @@ +13,5 @@
> > >  
> > >  labelDefaultFont=Default (%S)
> > >  
> > > +veryLargeMinimumFontTitle=Large minimum font size
> > > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> > 
> > veryLargeMinimumFontWarning=Some of the selected font sizes are known to
> > cause issues with &brandShortName;. Font sizes 24px or smaller are
> > recommended.
> > 
> > @@ +14,5 @@
> > >  labelDefaultFont=Default (%S)
> > >  
> > > +veryLargeMinimumFontTitle=Large minimum font size
> > > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> > > +acceptVeryLargeMinimumFont=Set minimum size
> > 
> > acceptVeryLargeMinimumFont=I understand, please keep the large font sizes
> > 
> > @@ +15,5 @@
> > >  
> > > +veryLargeMinimumFontTitle=Large minimum font size
> > > +veryLargeMinimumFontWarning=You have selected a very large minimum font size (more than 24 pixels). This may make it difficult or impossible to use some important configuration pages like this one. Do you really want to set such a large minimum size?
> > > +acceptVeryLargeMinimumFont=Set minimum size
> > > +reduceMinimumFont=Reduce these
> > 
> > reduceMinimumFont=Lower the affected font sizes to 24 pixels
> 
> While I like your suggestions, aren't these a bit long for button text?

Per discussion with Jared out-of-band, this should be "Keep my changes anyway" for the 'accept' button and just a Cancel option instead of the "reduce" option. Reminder to self to implement and re-request review. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Making the cancel bit work took longer than I thought... anyway, this seems to do it.
Attachment #8567092 - Flags: review?(jaws)
Attachment #8564351 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8567092 [details] [diff] [review]
warn about large fonts,

Review of attachment 8567092 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixed.

::: browser/components/preferences/fonts.js
@@ +134,5 @@
> +    let confirmLabel = strings.getString("acceptVeryLargeMinimumFont");
> +    let warningMessage = strings.getString("veryLargeMinimumFontWarning");
> +    let {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});
> +    let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_CANCEL |
> +                Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING;

The order of the buttons should be reversed for Windows. Windows uses [OK] [Cancel] but on OSX it is [Cancel] [OK]. I can't remember the order for Linux (but probably can mirror Windows).
Attachment #8567092 - Flags: review?(jaws) → review+
w/ button ordering fixed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/0af7b6d63a81
https://hg.mozilla.org/mozilla-central/rev/0af7b6d63a81
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1135628
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Gijs and I discussed this bug and decided on the following solution:
> 
> 1. File a new bug that will track making our in-content pages
> (about:preferences, about:config, etc) work better with larger font sizes
> (converting sizes from px to em).

Filed bug 1135628.
Blocks: 1135628
No longer depends on: 1135628
Hey Michael, do you think we should proactively get a page on SUMO for users who may have set their minimum font size to larger than 24px? Once in-content preferences is released, these users will have trouble using the in-content preferences to change this and other preferences.

These users will need to go to about:config and reset their font.minimum-size.* preferences to fix this (about:config is also affected but not as bad).
Flags: needinfo?(mverdi)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> Hey Michael, do you think we should proactively get a page on SUMO for users
> who may have set their minimum font size to larger than 24px? 

Let's ask Joni about the SUMO page (she's the support content manager).

> Once in-content preferences is released, these users will have trouble using the
> in-content preferences to change this and other preferences.
> 

I don't know much about this issue - do people set this larger than 24px so that they can read text? How will they still be able to do that if they have to make it smaller so that they can use the preferences?

> These users will need to go to about:config and reset their
> font.minimum-size.* preferences to fix this (about:config is also affected
> but not as bad).

Can we give people an easier way to fix this? I'm assuming refresh would work but seems like overkill. What about a restartless add-on that changes this one pref?
Flags: needinfo?(mverdi) → needinfo?(jsavage)
I don't think we have a good idea of how many people this would affect. Setting a minimum font size larger than 24px will probably make many webpages quite unusable. This is a different behavior than zooming in on webpages, which is less likely to break content and is probably used more often.
Since this is a temporary issue, we can write up a canned response in the support forums. We can also mention the about:config method in an existing article: https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages.

If affected users still aren't finding these solutions, we'll write a dedicated article.
Flags: needinfo?(jsavage)
You need to log in before you can comment on or make changes to this bug.