Closed Bug 623820 Opened 14 years ago Closed 13 years ago

"Reformat text" should set a minimum font size instead of textZoom, to reduce layout mangling

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Product:
Firefox for Android Graveyard
Component:
Panning/Zooming
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec2.0+)

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
fennec 2.0+ ---

People

(Reporter: tchung, Assigned: blassey)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: relnote)

Attachments

(4 files, 7 obsolete files)

text zoom reflow screenshot
77.29 KB, image/png
Details
patch
11.98 KB, patch
roc
: review+
dbaron
: review+
Details | Diff | Splinter Review
m-b patch
3.23 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
zoom screenshot 3/11 build
74.37 KB, image/png
Details 
When double tap zooming with textzoom reflow enabled, non mobile sites like mobile AMO does not scale correctly.

See screenshot

REpro:
1) install android trunk: Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre Fennec/4.0b4pre	
2) prefs > Enable reformat text on zoom
3) go to mobile AMO (see URL)
4) when page loads, double tap zoom outside the featured addons box
5) Verify zooming in page does not render site well

Expected:
- reflow on zoom scales correctly for non mobile site like AMO.   When option is off, there is no zoom

Actual:
- see screenshot
tracking-fennec: --- → ?
This is going to be an issue as long as we are using text zoom to reformat text.  Some sites don't layout well at very text sizes.

The biggest problems are in cases like comment 0, where you tap not in a specific page element but in the a background area, so that we try to make text readable while also fitting the entire page on-screen.  If you use the feature to zoom in to a specific column or paragraph, it generally works okay.

Fixing this the "right" way would probably involve re-implementing reflow in the layout engine (bug 578179).
Assignee: nobody → mbrubeck
tracking-fennec: ? → 2.0+
Unless someone has a suggestion for how to use text zoom without breaking any page layouts, I'm not sure we can fix this before version 4.0.  If we want to reflow text *without* text zoom, then I think we need to investigate bug 578179 (Android-style text reflow in the layout engine).
Depends on: 578179
We could bail when trying to zoom the <html> or <body>, if we don't already. Do we reset the text zoom if someone tries to pinch-zoom out of a double-tap zoom?
(In reply to comment #3)
> We could bail when trying to zoom the <html> or <body>, if we don't already.

That would break the usefulness of reflow on simple HTML pages that are just a <body> element and some text, where it's often needed most.

> Do we reset the text zoom if someone tries to pinch-zoom out of a double-tap
> zoom?

No, and we probably should.  Maybe when you get back to the default zoom level, or close to it.
Blocks: 611555
Depends on: font-inflation
Summary: Text zoom reflow doesnt scale correctly on AMO → Text zoom reflow messes up layout on AMO and other sites
the patches currently on bug 627842 make this much better, once we get that sorted out we'll follow up for reflow zoom in this bug
Attached patch patch for min font on a window (obsolete) — — Splinter Review 
Assignee: mbrubeck → blassey.bugs

        Attachment #515481 -
        Flags: review?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        m-b patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #515482 -
        Flags: review?(mbrubeck)

    
    

    
  
Comment on attachment 515482 [details] [diff] [review]
m-b patch

Unlike the previous behavior, this will potentially change the fonts even when zoomed out (since it still sets a minimum font size).  That's probably a good thing; just noting the change...

        Attachment #515482 -
        Flags: review?(mbrubeck) → review+

    
    

    
  
We don't have to do that. We could set the min font to 0 if the zoom factor is 1.0.

    
    

    
  
Comment on attachment 515482 [details] [diff] [review]
m-b patch

(In reply to comment #10)
> We don't have to do that. We could set the min font to 0 if the zoom factor is
> 1.0.

Hmm, we should probably do that - otherwise, the first page will load with minFontSize = 0, double-tapping will increase, and then double-tapping again will decrease not back to zero but instead to 720.

Instead of "if (aZoom == 1.0) viewer.minFontSize = 0", I would change the name of this function to _setMinFontSize, and change _setTextZoom(1) to _setMinFontSize(0).

If we *do* want to have a default minimum font size when zoomed out (a good idea, but could be done as a followup), then it should be a pref, and it should be set on pageshow rather than pagehide so it affects the first page load the same as later page loads.

        Attachment #515482 -
        Flags: review+ → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        m-b patch (obsolete)
        — 
          — Splinter Review
      

    


  
asking for mfinkle's review since this is based largely on a patch mbrubeck gave me

        Attachment #515482 -
        Attachment is obsolete: true

        Attachment #515502 -
        Flags: review?(mark.finkle)
Whiteboard: [has-patch][needs review]

    
    

    
  
+  // Set the min donr on all children of mContainer (even if our min font didn't

"donr"?

+SetExtResourceMinFontSize(nsIDocument* aDocument, void* aClosure)

External

+  if (pc && aMinFontSize != mPresContext->MinFontSize()) {
+      pc->SetMinFontSize(aMinFontSize)

Fix indent

Why are you changing nsComputedDOMStyle::GetLineHeightCoord?

dbaron should review this as well.

    
    

    
  
I assume the IDL changes will need special interfaces

    
    

    
  
(In reply to comment #13)

> Why are you changing nsComputedDOMStyle::GetLineHeightCoord?

My (most likely flawed) understanding of this is that we're changing the font size we'd need to reflect that in our line height calculations. Are you saying we don't?

    
    

    
  
nsDOMComputedStyle is used for DOM APIs that get layout information. It's not used for the actual layout.

    
    

    
  
We are removing this for Fennec 4 release. See bug 637881
tracking-fennec: 2.0+ → 2.0next+

    
    

    
  
Re-nominating for 4.0.  If bug 627842 proves too risky, I propose that we re-enable the reflow feature and fix this bug.
tracking-fennec: 2.0next+ → ?

    
    

    
  
re-plusing based on minusing of 627842
tracking-fennec: ? → 2.0+
Whiteboard: [has-patch][needs review] → [has-patch][needs review(mfinkle, roc)]

    
    

    
  
See comment #16. Is it really necessary to mess with line-height stuff?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #515481 -
        Attachment is obsolete: true

        Attachment #517972 -
        Flags: review?(roc)

        Attachment #515481 -
        Flags: review?(roc)

    
  
Blocks: 640217
No longer blocks: 640217

    
    

    
  
Hmm. I wonder if it's possible to reuse the minimum font size that nsRuleNode::ComputeFontData gets from prefs. Maybe your API should just override nsPresContext::mMinimumFontSize? And perhaps instead of calling GetCachedIntPref, we should create a proper API, say nsPresContext::GetMinimumFontSize.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Roc, is this what you were thinking?

        Attachment #518271 -
        Flags: review?(roc)

    
    

    
  
No. I was thinking that ComputeFontData would call nsPresContext::GetMinimumFontSize instead of GetCachedIntPref, which returns a value which is the maximum of the value set by your API and the pref min size. SetMinFontSize would share code with the code that updates things when the min-font-size pref changes.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #517972 -
        Attachment is obsolete: true

        Attachment #518271 -
        Attachment is obsolete: true

        Attachment #518517 -
        Flags: review?(roc)

        Attachment #517972 -
        Flags: review?(roc)

        Attachment #518271 -
        Flags: review?(roc)

    
    

    
  
Do you still need the change to nsStyleFont::ZoomText? I was thinking you wouldn't need it.

nsIDOMWindow.idl would need an IID rev, maybe this should be moved somewhere else? Or removed --- chrome script could use nsIMarkupDocumentViewer.idl? (which also needs an IID rev)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  

        Attachment #518517 -
        Attachment is obsolete: true

        Attachment #518599 -
        Flags: review?(roc)

        Attachment #518517 -
        Flags: review?(roc)

        Attachment #518599 -
        Flags: review?(roc)

        Attachment #518599 -
        Flags: review?(dbaron)

        Attachment #518599 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        m-b patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #515502 -
        Attachment is obsolete: true

        Attachment #518600 -
        Flags: review?(mark.finkle)

        Attachment #515502 -
        Flags: review?(mark.finkle)

    
    

    
  
Comment on attachment 518599 [details] [diff] [review]
patch

Remove kPresContext_MinimumFontSize and the code in GetCachedIntPref
that handles it, since it's now both unused and a bad idea to use.

>+  PRInt32 MinFontSize() const {
>+    return mMinFontSize > mMinimumFontSize ? mMinFontSize : mMinimumFontSize;
>+  }

Use NS_MAX.

I wonder whether nsPrintEngine::DoCommonPrint should set MinFontSize 
where it sets TextZoom and FullZoom.  I think it should;
DocumentViewerImpl::ReturnToGalleyPresenation is probably its opposite.

Same for DocumentViewerImpl::InitPresentationStuff.

In SetChildMinFontSize, no need for:
>+  NS_ENSURE_TRUE(branch,);
which will probably cause warnings or errors on some platforms.

>+  // Set the min donr on all children of mContainer (even if our min font didn't

Please wrap this comment at less than 80 characters.  Also,
s/donr/font/.

>+  if (pc && aMinFontSize != mPresContext->MinFontSize()) {
>+      pc->SetMinFontSize(aMinFontSize);
>+  }

Local style is 2-space indent.  (Yes, I know you copied code that was 
wrong.)

Please rename nsPresContext::mMinimumFontSize to mMinFontSizePref.


r=dbaron with that.

        Attachment #518599 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 518600 [details] [diff] [review]
m-b patch


>diff --git a/app/mobile.js b/app/mobile.js

> pref("browser.ui.zoom.reflow", false); // Change text wrapping on double-tap
> 
>+pref("browser.minFontSize.default", 540);

Why do we need the default size in a pref? Can't we hard code that as a const magic number in the JS?

>+pref("browser.minFontSize.zoom", 720);

Since I think we can drop the default pref, let's change the name of this one to "browser.ui.zoom.reflow.minFontSize" and move it right under "browser.ui.zoom.reflow"

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>       case "pagehide":
>+      if (aEvent.target == content.document)

indent

>+          this.pageInTransition = true;
>+      case "pageshow":
>         if (aEvent.target == content.document) {
>-          this._isZoomedToElement = false;
>-          this._setTextZoom(1);
>+          this.pageInTransition = false;
>+          this._resetFontSize();          
>         }
>         break;

I assume you want a "break" in the pagehide case, since without one you'll fall into the pageshow case and set pageInTransition back to false. My natural dislike for one-trick boolean flags ponies and the fact that the flag was always being set to false, makes me wonder if you can just drop it from the code for now. I'm not even sure how lucky you'd have to be to double tap a page after a pagehide event.

Hmm, yeah, given that revelation, I think you can remove the pageInTransition support altogether. It's way to edge case to be useful, imo.

Looks good, but I'd like to see another patch before checkin

        Attachment #518600 -
        Flags: review?(mark.finkle) → review+

    
    

    
  
(In reply to comment #30)
> >+pref("browser.minFontSize.default", 540);
> 
> Why do we need the default size in a pref? Can't we hard code that as a const
> magic number in the JS?
> 
> >+pref("browser.minFontSize.zoom", 720);
> 
> Since I think we can drop the default pref, let's change the name of this one
> to "browser.ui.zoom.reflow.minFontSize" and move it right under
> "browser.ui.zoom.reflow"

Adding the reflow feature broke the ability of addons like "Bigger Text" to change the font size in a simple way.  (It used to set the textZoom, but now the reflow code overwrites the textZoom on every pageshow and double-tap.)

Making this a pref would allow add-ons to change the default text size in a simple way without making intrusive changes to our zooming code.

    
    

    
  
Of course, now that reflow is using this new minFontSize thing, Bigger Text could set textZoom with impunity. But it would be better for Bigger Text to use minFontSize (for the same reason it's better for reflow).
Keywords: dev-doc-needed
Whiteboard: [has-patch][needs review(mfinkle, roc)] → [has-patch][needs review(mfinkle)]

    
    

    
  
OK, we can keep "browser.minFontSize.default" as a preference. It seems like
has some value. Just rename to "browser.ui.zoom.reflow.defaultFontSize".

These prefs are only used with the zoom reflow feature. Making them seem more
generic won't be helping anyone.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        m-b patch
        — 
          — Splinter Review
      

    


  

        Attachment #518600 -
        Attachment is obsolete: true

        Attachment #518635 -
        Flags: review?(mark.finkle)

    
    

    
  
Comment on attachment 518635 [details] [diff] [review]
m-b patch

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># Date 1299820224 18000
># Node ID 65895b2de8758c438318a153782389c86ea06792
># Parent  f773367cc208c7e050863aeefd396df709b54db5
>[mq]: min_font_size_pref
>
>diff --git a/app/mobile.js b/app/mobile.js
>--- a/app/mobile.js
>+++ b/app/mobile.js
>@@ -405,6 +405,7 @@ pref("browser.ui.kinetic.swipeLength", 1
> pref("browser.ui.zoom.pageFitGranularity", 9); // don't zoom to fit by less than 1/9 (11%)
> pref("browser.ui.zoom.animationDuration", 200); // ms duration of double-tap zoom animation
> pref("browser.ui.zoom.reflow", false); // Change text wrapping on double-tap
>+pref("browser.ui.zoom.reflow.minFontSize", 720);

Add the default value here too, so add-ons can safely tweak min font size and not get overwritten

pref("browser.ui.zoom.reflow.defaultFontSize", 0);

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>+    addEventListener("pageshow", this, false);

No need for this

>       case "pagehide":
>         if (aEvent.target == content.document) {
>-          this._isZoomedToElement = false;
>-          this._setTextZoom(1);
>+          this._resetFontSize();          
>         }

Remove the { } too

>       case "Browser:ZoomToPoint": {
>         let rect = null;
>         if (this._isZoomedToElement) {
>-          this._isZoomedToElement = false;
>-          this._setTextZoom(1);
>+          this._resetFontSize();
>         } else {

but keep these { }

>+  _resetFontSize: function _resetFontSize() {
>+    this._isZoomedToElement = false;
>+    this._setMinFontSize(0);

Grab the default from the pref, like your old patch

        Attachment #518635 -
        Flags: review?(mark.finkle) → review+

    
    

    
  
pushed https://hg.mozilla.org/mobile-browser/rev/0e67c1d45d06
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
    

    
  
From a developer docs standpoint, what needs documenting here? The only thing I see that looks like a likely candidate is the browser.ui.zoom.reflow.fontSize preference, but the default value of 720 confuses me. :)

    
    

    
  
I was thinking that the new nsIMarkupDocumentViewer_MOZILLA_2_0_BRANCH and its minFontSize attribute needed documentation. If it doesn't, then nevermind
Whiteboard: [has-patch][needs review(mfinkle)]

    
    

    
  
Odd. Those aren't defined in the patch checked in here, so I assumed they were old news. I'll look into it further.

    
    

    
  
(In reply to comment #40)
> Odd. Those aren't defined in the patch checked in here, so I assumed they were
> old news. I'll look into it further.

They're in the mozilla-central patch which was pushed earlier (comment 33): http://hg.mozilla.org/mozilla-central/rev/d4f00dcf1bb8

    
    

    
  


  
i'm still seeing display issues on the 3/11 build.   

Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre Fennec/4.0b6pre

    
    

    
  
Added documentation:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMarkupDocumentViewer

This interface wasn't documented at all, so I created this page outright. It has a couple of gaps, and could use a once-over to be sure it's accurate.

    
    

    
  
meant to reopen this unless this fix didnt make the 3/11 build.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to comment #44)
> meant to reopen this unless this fix didnt make the 3/11 build.

I'm not sure there is a 100% fix to this issue. We are trying to be better than before.

* What are you zooming into?

    
    

    
  
(In reply to comment #45)
> (In reply to comment #44)
> > meant to reopen this unless this fix didnt make the 3/11 build.
> 
> I'm not sure there is a 100% fix to this issue. We are trying to be better than
> before.
> 
> * What are you zooming into?

i was zooming into spaces outside of the body of text.
(eg. right of the NewYork Presbyterian banner on this image: http://tinypic.com/view.php?pic=ng1vtd&s=7).  

per irc, i was notified that modifying layout of pages is expected to mess up now.  As long as body of the article can text zoom appropriately, i'm fine with not blocking2.0

    
    

    
  
I'm going to re-resolve this bug for now; let's use followup bugs for cases that still aren't handled well.  I'll open one up for the case of zooming into the root element of a multi-column document (as in the nytimes and some of the AMO cases above).
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Summary: Text zoom reflow messes up layout on AMO and other sites → "Reformat text" should set a minimum font size instead of textZoom, to reduce layout mangling

    
    

    
  
If there's not much left that can be done with layout cases, i suggest relnote this so users are aware of text zoom reflow behavior.

Another example:
zoom in on prices here, side banner ad will cut it off: http://i.imgur.com/IKW3j.png
Whiteboard: relnote
Depends on: 641075

    
    

    
  
Filed followup bug 641075.

    
    

    
  
(In reply to comment #48)
> zoom in on prices here, side banner ad will cut it off:
> http://i.imgur.com/IKW3j.png

Yup, same thing happens if you use text zoom in desktop Firefox (turn off browser.zoom.full).  This case and the NYTimes case should be fixed by bug 623820, which was not ready for this release but is marked blocking 2.0next+.

    
    

    
  
Verified fix directly for body text zoom.  Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre Fennec/4.0b6pre
Status: RESOLVED → VERIFIED
No longer depends on: 641075
Depends on: 646180

    
  
Depends on: 687297

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: