Closed Bug 687297 Opened 13 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: maxaks, Assigned: kip)

References

Details

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

Attachments

(5 files, 4 obsolete files)

ISO-8859-1.html
286 bytes, text/html
Details
Shift_JIS.html
285 bytes, text/html
Details
UTF-8.html
281 bytes, text/html
Details
V2 proposed fix for bug 687297
7.60 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Mochitest for Bug 687297
6.75 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Attached file shift-jis (obsolete) — 
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.
Attachment #560743 - Attachment mime type: text/plain → text/html
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
Attached file ISO-8859-1.html — 

    
    

    
  

      
      
      
      

        Attached file
          
        Shift_JIS.html
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        UTF-8.html
        — 
      

    


  

    
    

    
  
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>sdrs</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.

    
    

    
  
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++]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        V1 proposed fix for bug 687297 (obsolete)
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  


  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug687297_test.patch (obsolete)
        — 
          — Splinter Review
      

    


  
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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Mochitest for Bug 687297 (obsolete)
        — 
          — Splinter Review
      

    


  
bug687297_test.patch has been updated based on David Baron's review feedback.

        Attachment #8382531 -
        Attachment is obsolete: true

        Attachment #8382626 -
        Flags: review?(dbaron)

    
    

    
  
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.

    
    

    
  


  
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)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d86dd791088b
https://hg.mozilla.org/mozilla-central/rev/d30b9a757fc2
Assignee: nobody → kgilbert
Status: NEW → RESOLVED
Closed: 10 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: