language-specific minimum font size is propagated to pages of different language in same window

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: maxaks, Assigned: kip)

Tracking

({regression})

Trunk
mozilla30
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=dbaron][lang=c++])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 560743 [details]
shift-jis

Using the following settings:
- Minimum font size for Western languages: None
- Minimum font size for Japanese: 24

Visit a Shift-JIS encoded page (the one in the attachment for example), then, in the same tab, open any other non-Shift-JIS page: the minimum font size of 24 is carried over for the rest of the session, regardless of the encoding of the page.
(Reporter)

Updated

7 years ago
Attachment #560743 - Attachment mime type: text/plain → text/html

Comment 1

7 years ago
Confirmed 
http://hg.mozilla.org/mozilla-central/rev/85fb038d1dd1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110917 Firefox/9.0a1 ID:20110917121100

Regression window(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/c56df99c1c75
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre ID:20110310150634
Fails:
http://hg.mozilla.org/mozilla-central/rev/d4f00dcf1bb8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre ID:20110310203732
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c56df99c1c75&tochange=d4f00dcf1bb8

Regressed by:
d4f00dcf1bb8	Brad Lassey — bug 623820 - Text zoom reflow messes up layout on AMO and other sites r=roc,dbaron a=blocking-fennec
Blocks: 623820
Status: UNCONFIRMED → NEW
Component: Preferences → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: preferences → layout

Comment 2

7 years ago
Created attachment 560750 [details]
ISO-8859-1.html

Comment 3

7 years ago
Created attachment 560751 [details]
Shift_JIS.html

Comment 4

7 years ago
Created attachment 560752 [details]
UTF-8.html

Comment 5

7 years ago
Comment on attachment 560743 [details]
shift-jis

><!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
><html>
><head>
><meta http-equiv="Content-Type" content="text/html; charset=Shift_JIS">
><title>‚s‚d‚r‚s</title>
></head>
><body>
>‚“‚•‚
></body>
></html>
Attachment #560743 - Attachment is obsolete: true
Version: 5 Branch → Trunk
Bug 623820 was about propagating the minimum font size from one page to the next in the same tab. I guess it's not clear how that should interact with per-script minimum font size settings. Maybe we should only propagate the minimum font size when pages have the same script.
Duplicate of this bug: 902092
I think we should be propagating the set-from-the-outside min font size from page to page, but not the max of that with the language-specific pref.  (See also comments I made in bug 902092.)
This should be fixed by revising the fix to bug 623820 (https://hg.mozilla.org/mozilla-central/rev/d4f00dcf1bb8) by:

 * clearly separating the min font size from the pref, the extra min font size from the container, and their combination, for example, by renaming mMinFontSize -> mExtraMinFontSize, SetMinFontSize -> SetExtraMinFontSize, etc.  (Maybe "Extra" isn't the best name, but I haven't yet thought of a better one.)

 * adding a getter for the extra min font size in addition to the getter for their combination

 * making DocumentViewerImpl::GetMinFontSize and TransferZoomLevels in nsDocument.cpp call that new getter instead of the getter for the combination.  (Also, please check to see if there are other new callers.)

 * adding an automated test for this bug (probably a mochitest)
Whiteboard: [mentor=dbaron]
OS: Windows 7 → All
Hardware: x86 → All
Summary: Minimum font size setting carried over after visiting a non UTF-8 encoded page → language-specific minimum font size is propagated to pages of different language in same window
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9)
>  * clearly separating the min font size from the pref, the extra min font
> size from the container, and their combination, for example, by renaming
> mMinFontSize -> mExtraMinFontSize, SetMinFontSize -> SetExtraMinFontSize,
> etc.  (Maybe "Extra" isn't the best name, but I haven't yet thought of a
> better one.)
> 
>  * adding a getter for the extra min font size in addition to the getter for
> their combination

Oops, I should have been clearer that for these two points, I'm talking specifically about the code in nsPresContext.
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
(Assignee)

Comment 11

5 years ago
Created attachment 8381571 [details] [diff] [review]
V1 proposed fix for bug 687297

V1 Fix: Bug 687297 - language-specific minimum font size is propagated to pages of different language in same window

To correct this bug, the minimum font size is propagated to pages without first being max’ed with the language-specific minimum font size preference.

A separate accessor has been added to nsPresContext to enable TransferZoomLevels and nsDocumentViewer::GetMinFontSize to access the minimum font size that has not been Max’ed against the language specific minimum font size preset.

nsPresContext::mMinFontSize, its getter, and its setter have been renamed to more clearly distinguish them from minFontSize used elsewhere.
Attachment #8381571 - Flags: review?(dbaron)
Comment on attachment 8381571 [details] [diff] [review]
V1 proposed fix for bug 687297

>V1 Fix: Bug 687297 - language-specific minimum font size is propagated to pages of different language in same window

Drop the "V1 Fix: ", and change the rest of the line to describe the fix rather than describing the bug.


>   /**
>    * Get the minimum font size for the specified language. If aLanguage
>-   * is nullptr, then the document's language is used.
>+   * is nullptr, then the document's language is used.  This is the
>+   * effective minimum font size, taking into consideration the
>+   * language font size preference.
>    */

Instead of the second sentence, I'd say something more like "This combines
the language-specific global preference with the per-presentation base
minimum font size."


>+  /**
>+   * Get the base minimum font size.  This size is independent of the
>+   * language font size preference.
>+   */

Perhaps make this comment describe both the getter and the setter?


>-
>-    mMinFontSize = aMinFontSize;
>+    
>+    mBaseMinFontSize = aMinFontSize;

Don't insert trailing whitespace.

>-  int32_t               mMinFontSize;   // Min font size, defaults to 0
>-  float                 mTextZoom;      // Text zoom, defaults to 1.0
>-  float                 mFullZoom;      // Page zoom, defaults to 1.0
>+  // Base minimum font size, independent of the language font size preference. Defaults to 0
>+  int32_t               mBaseMinFontSize;
>+  
>+  float                 mTextZoom;          // Text zoom, defaults to 1.0
>+  float                 mFullZoom;          // Page zoom, defaults to 1.0

Leave the comment indentation alone, and don't insert trailing whitespace.


Looks good with those things addressed.  You should address them,
and upload a new patch (including "r=dbaron" at the end of the first
line of the commit message).

Also see https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

But this also shouldn't land until you also have the test.
Attachment #8381571 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 8382430 [details] [diff] [review]
V2 proposed fix for bug 687297

Updated with dbaron's review feedback.
Attachment #8382430 - Flags: review?(dbaron)
Comment on attachment 8382430 [details] [diff] [review]
V2 proposed fix for bug 687297

r=dbaron

(but normally if you get review+, you don't need to ask for review again if you've done what the comments ask for.  You should also mark the previous patch as obsolete, though if you use hg bzexport, bzexport will do that for you.)
Attachment #8382430 - Flags: review?(dbaron) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 8382531 [details] [diff] [review]
bug687297_test.patch

Added a Mochitest for this bug
Attachment #8381571 - Attachment is obsolete: true
Attachment #8382531 - Flags: review?(dbaron)
Comment on attachment 8382531 [details] [diff] [review]
bug687297_test.patch

A few things:  I think you should use SpecialPowers.pushPrefEnv rather than SetIntPref, because preference changes are asynchronous on platforms that have Web content in a separate process ("electrolysis", "e10s").  This is similar, but uses a callback for when the pref change is known to be done.

I think it's actually also worth testing that the middle frame has a different size.  Given that you have a test for the functionality, may as well test the things that happen.  (It might be easier to have each frame cal a function on the parent rather than pass information around in window.name.)

Also, try to not end up with "No newline at end of file"; hg uses the Unix convention of LF as line *terminator* (rather than the Windows convention of CR+LF as line *separator*).  On some platforms editors might tend to produce files without the final newline.

Otherwise this looks good, though.  (Does it appear in a separate window or in a separate tab?)  Probably worth pushing to try server to see that it works across platforms, though.
Attachment #8382531 - Flags: review?(dbaron) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 8382626 [details] [diff] [review]
Mochitest for Bug 687297

bug687297_test.patch has been updated based on David Baron's review feedback.
Attachment #8382531 - Attachment is obsolete: true
Attachment #8382626 - Flags: review?(dbaron)
Comment on attachment 8382626 [details] [diff] [review]
Mochitest for Bug 687297

r=dbaron
Attachment #8382626 - Flags: review?(dbaron) → review+
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=58c06fbea0a1
(against a rather current mozilla-inbound, and thus including a bunch of revisions that hadn't been pushed to try by anyone before)
https://tbpl.mozilla.org/?rev=b0e36a2e2ee5&tree=Mozilla-Inbound shows the failure was only in the B2G desktop builds, which is interesting.  Although I wouldn't necessarily assume the test was running in the B2G emulator builds, since there's a lot of out-of-band test disabling for b2g.

Debugging that failure looks painful; I'm not sure it's necessarily worthwhile on its own, but it probably is as part of making B2G desktop tests more reliable.
(Assignee)

Comment 23

5 years ago
Created attachment 8383782 [details] [diff] [review]
Mochitest for Bug 687297

The Mochitest for this bug is now excluded from the B2G Desktop Client tests, due to bug 948948.
Attachment #8382626 - Attachment is obsolete: true
Attachment #8383782 - Flags: review?(dbaron)
Comment on attachment 8383782 [details] [diff] [review]
Mochitest for Bug 687297

r=dbaron
Attachment #8383782 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d86dd791088b
https://hg.mozilla.org/mozilla-central/rev/d30b9a757fc2
Assignee: nobody → kgilbert
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.