Closed Bug 759755 Opened 12 years ago Closed 12 years ago

Links on some web pages are rendered misplaced

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
blocking-fennec1.0 --- +

People

(Reporter: ehsan.akhgari, Assigned: dbaron)

References

Details

(Whiteboard: [readability])

Attachments

(8 files, 2 obsolete files)

screenshot
121.02 KB, image/png
Details
patch 3
4.57 KB, patch
roc
: review+
mfinkle
: approval-mozilla-aurora+
mfinkle
: approval-mozilla-beta+
Details | Diff | Splinter Review
patch 4: make svg:foreignObject considered a constrained height
1.33 KB, patch
roc
: review+
mfinkle
: approval-mozilla-aurora+
mfinkle
: approval-mozilla-beta+
Details | Diff | Splinter Review
patch 2
7.97 KB, patch
mfinkle
: approval-mozilla-aurora+
mfinkle
: approval-mozilla-beta+
Details | Diff | Splinter Review
interdiff against previous reviewed patch 2
3.63 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Screenshot
59.55 KB, image/png
Details
Screenshot 2
87.60 KB, image/png
Details
patch 5
2.58 KB, patch
bzbarsky
: review+
mfinkle
: approval-mozilla-aurora+
mfinkle
: approval-mozilla-beta+
Details | Diff | Splinter Review 
See the first few links on http://matt.might.net/articles/my-sons-killer/ on Aurora.

It might be because the links and the rest of the text are rendered in slightly different sizes...
Summary: Links on web pages are rendered misplaced → Links on some web pages are rendered misplaced
Ehsan can you try with the font size set to 0?  (Font inflation turned off)
Please to screenshot, sir.
(In reply to Naoki Hirata :nhirata from comment #1)
> Ehsan can you try with the font size set to 0?  (Font inflation turned off)

How do I do that?
Attached image screenshot — 

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> (In reply to Naoki Hirata :nhirata from comment #1)
> > Ehsan can you try with the font size set to 0?  (Font inflation turned off)
> 
> How do I do that?

Oops.  I meant tiny.

Hit the menu button -> more -> settings
Pan to Text size, change to Tiny

This turns font inflation off.  Medium is default.

    
    

    
  
Could you also place which build you have installed please?

    
    

    
  
I do not see this issue on the Samsung Galaxy S Captivate w/ today's nightly build 5/30/2012

    
    

    
  
It doesn't seem to happen with font inflation turned off.  I'm using the latest Aurora.

Note that sometimes you have to play around with the link a bit, like double tap on the link or around, and zoom in and back a few times.  It doesn't happen every time.
blocking-fennec1.0: ? → +
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Whiteboard: [readability]

    
    

    
  
This looks OK for me on Nightly. Can you try it on Nightly?

    
    

    
  
I just saw this on today's nightly.

    
    

    
  
I think the conditions for reproducing relate to text run cache expiration.

    
    

    
  
likely regressio from bug 747720, I think

    
    

    
  
(In reply to David Baron [:dbaron] from comment #12)
> likely regression from bug 747720, I think

So the reason I say this is that bug 747720 increased the scope of the area we use to determine what width to use for font inflation, but didn't add a mechanism that forces a destructive enough reflow when that width changes.  (Additionally, residue from bug 706193:  I think we have the same problem for when we change from having font inflation enabled or disabled.)

Since we don't necessarily reflow at all, I think, we end up holding on to the old text runs until they expire, but then once they expire when we regenerate new text runs we generate them at the correct new size (and potentially paint with the new size without ever having reflowed with it).

    
    

    
  
David: We're keeping this on the blocker list based on our last conversation about it. Can you comment on fix risk for this one?
Assignee: nobody → dbaron

    
    

    
  
(In reply to David Baron [:dbaron] from comment #14)
> (In reply to David Baron [:dbaron] from comment #12)
> > likely regression from bug 747720, I think
> 
> So the reason I say this is that bug 747720 increased the scope of the area
> we use to determine what width to use for font inflation, but didn't add a
> mechanism that forces a destructive enough reflow when that width changes. 
> (Additionally, residue from bug 706193:  I think we have the same problem
> for when we change from having font inflation enabled or disabled.)

Is it possible to make the change that would force a more destructive reflow, or do we want to avoid this for performance reasons?

> Since we don't necessarily reflow at all, I think, we end up holding on to
> the old text runs until they expire, but then once they expire when we
> regenerate new text runs we generate them at the correct new size (and
> potentially paint with the new size without ever having reflowed with it).

How often does the text run cache expire? Is it on the order of ms, or are we talking only when we need to repaint? If it's the latter, could we force a reflow if font inflation is enabled to overcome this issue?

    
    

    
  
(In reply to Scott Johnson (:jwir3) from comment #16)
> (In reply to David Baron [:dbaron] from comment #14)
> > (In reply to David Baron [:dbaron] from comment #12)
> > > likely regression from bug 747720, I think
> > 
> > So the reason I say this is that bug 747720 increased the scope of the area
> > we use to determine what width to use for font inflation, but didn't add a
> > mechanism that forces a destructive enough reflow when that width changes. 
> > (Additionally, residue from bug 706193:  I think we have the same problem
> > for when we change from having font inflation enabled or disabled.)
> 
> Is it possible to make the change that would force a more destructive
> reflow, or do we want to avoid this for performance reasons?

I think it should be fine.

> > Since we don't necessarily reflow at all, I think, we end up holding on to
> > the old text runs until they expire, but then once they expire when we
> > regenerate new text runs we generate them at the correct new size (and
> > potentially paint with the new size without ever having reflowed with it).
> 
> How often does the text run cache expire? Is it on the order of ms, or are
> we talking only when we need to repaint? If it's the latter, could we force
> a reflow if font inflation is enabled to overcome this issue?

I think it should be after ~30 seconds of non-use (which I think means no reflow or repaint).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 2 (obsolete)
        — 
          — Splinter Review
      

    


  
I think this should fix it, though I haven't yet tested it.

        Attachment #630776 -
        Flags: review?(roc)

    
    

    
  
Please test build in #19
Keywords: qawanted

    
    

    
  
(I'd put it there mainly so I could get to it... except then my phone decided it doesn't like wifi or 3G anymore, and even rebooting doesn't fix it.)

    
    

    
  
So I managed to repro the problem in the build with the fix; and I think I've figured out how to repro more reliably:  in particular, by swapping between landscape and portrait and then tapping links enough to get a hover effect but not to activate them.  Those steps to repro also suggest a different underlying problem from what I originally thought the problem was.  (I still need to test if the patch here fixes bug 757179...)

    
    

    
  
... though I'd have thought that problem would have been fixed by bug 747231.

    
    

    
  
This patch does fix bug 757179 in that it makes the display consistent, though we end up being consistent on the less preferred behavior.

    
    

    
  
Er, maybe it doesn't, actually.

    
    

    
  
Er, wait, the problem is that I need a change (for both this bug and bug 747231) that's equivalent to
#define nsChangeHint_ReflowFrame                        \
  nsChangeHint(nsChangeHint_NeedReflow |                \
               nsChangeHint_ClearAncestorIntrinsics |   \
               nsChangeHint_ClearDescendantIntrinsics | \
               nsChangeHint_NeedDirtyReflow)
but I'm actually only doing one equivalent to (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow).

        Attachment #630776 -
        Attachment description: patch → patch 2

        Attachment #631198 -
        Attachment description: patch, part 2 → patch 3

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 2 (obsolete)
        — 
          — Splinter Review
      

    


  
Two changes to patch 2:

To fix the test failures I was hitting, special-case svg:foreignObject frames, which use dirty bits differently.

Also, remove the existing code that this code should be replacing.

        Attachment #630776 -
        Attachment is obsolete: true

        Attachment #631228 -
        Flags: review?(roc)

    
    

    
  


  
And while I have svg:foreignObject in my head, they should be considered a constrained height.

        Attachment #631229 -
        Flags: review?(roc)

    
    

    
  
(In reply to David Baron [:dbaron] from comment #30)
> In a few hours there will hopefully be a new test build at:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.
> com-730051dbca6f/try-android/fennec-16.0a1.en-US.android-arm.apk

so this still doesn't seem to fix the steps in comment 24

    
    

    
  
So I believe these patches are correct, and they fix the randomness on sfgate.com.  They don't fix this bug because bug 747231 has regressed.

I'm planning to land these patches and then investigate why bug 747231 has regressed.

    
    

    
  
New try build with all the patches here, on the same baseline as today's nightly, in case there was something odd about my previous baseline:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-6d87e10e7e35/try-android/fennec-16.0a1.en-US.android-arm.apk

    
    

    
  
(In reply to David Baron [:dbaron] from comment #35)
> So I believe these patches are correct, and they fix the randomness on
> sfgate.com.  They don't fix this bug because bug 747231 has regressed.

Well, now I can no longer repro the bug 747231 having regressed, either on nightly or on my try builds.

    
    

    
  
Backed out patches 2 and 3:
https://hg.mozilla.org/mozilla-central/rev/af2a59c23347
for causing reftest failures (no inflation at all in the test) for of layout/reftests/font-inflation/container-with-clamping.html .  I think the problem is that the updating code in patch 2 needs to handle the clamping too, as perhaps do other things (though it's disturbing that we're initially reflowing iframes at an incorrect size in some cases).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 2
        — 
          — Splinter Review
      

    


  
Revised to fix (I hope) the intermittent orange from the previous landing.  The essential part of the revision is the addition of the:
      || (!frame->GetParent() && isHResize)
in nsHTMLReflowState::InitResizeFlags and the adjustment of the comment in the paragraph below it (which was out of date in the previous revision of the patch, but is now adjusted both for that and for this revision, and which should explain the need for the revision -- essentially, continuing to correctly handle dynamic changes for the test added in bug 743817).  (The horizontal resize check was also refactored into the |isHResize| variable since it is now used twice.)

        Attachment #631228 -
        Attachment is obsolete: true

        Attachment #631801 -
        Flags: review?(roc)

    
    

    
  
(In reply to David Baron [:dbaron] from comment #40)
> correctly handle dynamic changes for the test added in bug 743817).  (The

Er, sorry, I meant bug 707855.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #39)
> Backed out patches 2 and 3:
> https://hg.mozilla.org/mozilla-central/rev/af2a59c23347
> for causing reftest failures (no inflation at all in the test) for of
> layout/reftests/font-inflation/container-with-clamping.html 

Even after the backout there have been regular failures on inbound in container-with-clamping.html. Not sure if the backout had conflicts, or if Ryan's merge from m-c to inbound broke it somehow?

    
  
Depends on: 763434

    
    

    
  
Re comment #19; do we have a newer build here for testing?

    
    

    
  
(In reply to Ed Morley [:edmorley] from comment #42)
> Even after the backout there have been regular failures on inbound in
> container-with-clamping.html. Not sure if the backout had conflicts, or if
> Ryan's merge from m-c to inbound broke it somehow?

Turned out to be a bad merge (https://hg.mozilla.org/integration/mozilla-inbound/rev/75b67011b798) that reverted the backout amongst other things.

Re-backed out by:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7160c7286c4f

    
    

    
  
Comment on attachment 632005 [details] [diff] [review]
interdiff against previous reviewed patch 2

r=me

        Attachment #632005 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
qawanted - SV, Moz: please test this bug with the tinderbox m-c build in comment 48.  i think the testcase is in comment 0, unless there's another testcase i've overlooked in this bug.

    
    

    
  
AFAIK, we're trying to figure this out before 5 PM PDT.
Testing around : bug 747231 would be a good idea as well.

    
    

    
  

      
      
      
      

        Attached image
          
        Screenshot
        — 
      

    


  
Using the TB build from the earlier comment... this still occurs on this page. 

Load the webpage in landscape and change the orientation to portrait.
Scroll mid way.  (This is on a Galaxy S II)

    
    

    
  

      
      
      
      

        Attached image
          
        Screenshot 2
        — 
      

    


  
I showed blassey and it got worse when going to the home screen and then hitting the back button to go back to the web page.  see screenshot.

    
    

    
  
Yeah, I know that the example in this bug isn't fixed yet (hence this bug not being marked fixed yet); a bunch of the others should be, though.

    
    

    
  
1. go to http://matt.might.net/articles/my-sons-killer/ in landscape
2. change to portrait
3. click on the tab icon and create a new tab
4. hit the back button
5. pinch zoom.

    
    

    
  
Actually, that seems to fix the problem.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 5
        — 
          — Splinter Review
      

    


  
I'm hoping Boris might be able to review this soon, though I think it's relatively self-explanatory (at least with a drop more context, i.e., the whole function).

        Attachment #632110 -
        Flags: review?(bzbarsky)

    
    

    
  
In any case, I think others could probably review this as well if Boris isn't around.  I'm going to be at dinner for a bit; feel free to call/text my cell (in the internal phonebook) if you have questions.

    
    

    
  
Comment on attachment 632110 [details] [diff] [review]
patch 5

s/calld/called/ and r=me

        Attachment #632110 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
(In reply to David Baron [:dbaron] from comment #48)
> https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-
> central-android/1339448306/fennec-16.0a1.en-US.android-arm.apk is a
> mozilla-central build with the above patches

I used this build on my Galaxy Nexus.  Set font size to Large.   Rotation and tapping on the link will grow and shrink the link size.

See screencast for specifics: http://youtu.be/Vd1ddSDt2AA

    
    

    
  
(In reply to Tony Chung [:tchung] from comment #60)
> (In reply to David Baron [:dbaron] from comment #48)
> > https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-
> > central-android/1339448306/fennec-16.0a1.en-US.android-arm.apk is a
> > mozilla-central build with the above patches
> 
> I used this build on my Galaxy Nexus.  Set font size to Large.   Rotation
> and tapping on the link will grow and shrink the link size.
> 
> See screencast for specifics: http://youtu.be/Vd1ddSDt2AA

Yes, but that should be fixed in the build from comment 55.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/131961e5e0d1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

    
    

    
  
Comment on attachment 631801 [details] [diff] [review]
patch 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 747720

User impact if declined:  Font size inflation produces inconsistent results depending on things like packet or coalescing boundaries during incremental loading of pages (i.e., we do the font inflation based on a width that was correct at the time a piece of text was first displayed and don't update it when that width changes for later loading.  This means we may, for example, end up with inconsistent text sizes where the basic algorithms we use would lead to consistent ones.  This also makes it harder to describe and test bugs when behavior is inconsistent.

Testing completed (on m-c, etc.): on mozilla-central

Risk to taking this patch (and alternatives if risky): 
 * since it's invalidating more data, there is a performance cost.  I think it's unlikely to be problematic, though.
 * since it's making our behavior more consistent (i.e., better at responding to dynamic changes, and better at doing what the code is trying to do), there will be cases where currently we're exhibiting a mix of two behaviors (e.g., one display from incrementally loading, another when reaching a page from back/forward) where we become consistent in doing the one we like less

String or UUID changes made by this patch: none

        Attachment #631801 -
        Flags: approval-mozilla-beta?

        Attachment #631801 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 631198 [details] [diff] [review]
patch 3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 747231

User impact if declined: When things that should change our font size inflation calculations happen (such as incremental loading of a page, dynamic changes to a page's content, or switches between landscape/portrait orientation), we don't invalidate enough, and, in particular, end up failing to change font sizes and sizes of form controls.  However, because of the timer-based text run cache expiration, we will update the font sizes randomly at some later point, though this update will affect painting but not layout (causing widely spaced or overlapping text).

Testing completed (on m-c, etc.): on mozilla-central

Risk to taking this patch (and alternatives if risky): same as in comment 63

String or UUID changes made by this patch: none

        Attachment #631198 -
        Flags: approval-mozilla-beta?

        Attachment #631198 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 631229 [details] [diff] [review]
patch 4: make svg:foreignObject considered a constrained height

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 627842
User impact if declined: Hidden or overlapping text because we tried to apply font inflation in a context where we shouldn't have (inside an svg:foreignObject).
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): very low, since svg:foreignObject is not widely used on the Web.
String or UUID changes made by this patch: none

        Attachment #631229 -
        Flags: approval-mozilla-beta?

        Attachment #631229 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 632110 [details] [diff] [review]
patch 5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 747231
User impact if declined: Switch between landscape and portait doesn't cause the appropriate recalculation of font size inflation, but that calculation then happens randomly following user interaction (see, e.g., the video in comment 60).  In particular, the fix for bug 747231 is working for some pages but not others; this fix makes the code more reliable.
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): Potential risk to performance since we're doing more work to respond to a change between landscape and portrait, though it would just cause the code that we're already running on some pages to run on all of them.  (I'm not sure what the condition is that distinguishes those where the fix to bug 747231 works already and those where this change is required.)
String or UUID changes made by this patch: none

        Attachment #632110 -
        Flags: approval-mozilla-beta?

        Attachment #632110 -
        Flags: approval-mozilla-aurora?

        Attachment #631198 -
        Flags: approval-mozilla-beta?

        Attachment #631198 -
        Flags: approval-mozilla-beta+

        Attachment #631198 -
        Flags: approval-mozilla-aurora?

        Attachment #631198 -
        Flags: approval-mozilla-aurora+

        Attachment #631229 -
        Flags: approval-mozilla-beta?

        Attachment #631229 -
        Flags: approval-mozilla-beta+

        Attachment #631229 -
        Flags: approval-mozilla-aurora?

        Attachment #631229 -
        Flags: approval-mozilla-aurora+

        Attachment #631801 -
        Flags: approval-mozilla-beta?

        Attachment #631801 -
        Flags: approval-mozilla-beta+

        Attachment #631801 -
        Flags: approval-mozilla-aurora?

        Attachment #631801 -
        Flags: approval-mozilla-aurora+

        Attachment #632110 -
        Flags: approval-mozilla-beta?

        Attachment #632110 -
        Flags: approval-mozilla-beta+

        Attachment #632110 -
        Flags: approval-mozilla-aurora?

        Attachment #632110 -
        Flags: approval-mozilla-aurora+
Depends on: 764354
Depends on: 764497

    
    

    
  
kevin: please take a look to see if its fixed, thanks

    
    

    
  
This seems fixed on the Galaxy S II using Nightly 06/18/2012 build.  Leaving for review from kbrosnan.

    
    

    
  
Removing QA wanted and placing verified from comment 69, and tomorrow being release.
Status: RESOLVED → VERIFIED
Keywords: qawanted

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: