Links on some web pages are rendered misplaced

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: dbaron)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, blocking-fennec1.0 +)

Details

(Whiteboard: [readability])

Attachments

(8 attachments, 2 obsolete attachments)

121.02 KB, image/png
Details
4.57 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.97 KB, patch
Details | Diff | Splinter Review
3.63 KB, patch
Details | Diff | Splinter Review
59.55 KB, image/png
Details
87.60 KB, image/png
Details
2.58 KB, patch
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?
Created attachment 628475 [details]
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?
(Assignee)

Comment 10

5 years ago
I just saw this on today's nightly.
(Assignee)

Comment 11

5 years ago
I think the conditions for reproducing relate to text run cache expiration.
(Assignee)

Comment 12

5 years ago
likely regressio from bug 747720, I think
(Assignee)

Comment 13

5 years ago
maybe related to bug 757179
(Assignee)

Comment 14

5 years ago
(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).

Comment 15

5 years ago
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?
(Assignee)

Comment 17

5 years ago
(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).
(Assignee)

Comment 18

5 years ago
Created attachment 630776 [details] [diff] [review]
patch 2

I think this should fix it, though I haven't yet tested it.
Attachment #630776 - Flags: review?(roc)
Attachment #630776 - Flags: review?(roc) → review+
(Assignee)

Comment 19

5 years ago
Test build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-35e1923c3ba1/try-android/fennec-16.0a1.en-US.android-arm.apk

Comment 20

5 years ago
Please test build in #19
Keywords: qawanted
(Assignee)

Comment 21

5 years ago
(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.)
(Assignee)

Comment 22

5 years ago
Landed anyway:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e441f7ad9d
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cca15a0b49f3 for https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=30e441f7ad9d
(Assignee)

Comment 24

5 years ago
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...)
(Assignee)

Comment 25

5 years ago
... though I'd have thought that problem would have been fixed by bug 747231.
(Assignee)

Comment 26

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

Comment 27

5 years ago
Er, maybe it doesn't, actually.
(Assignee)

Comment 28

5 years ago
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).
(Assignee)

Comment 29

5 years ago
Created attachment 631198 [details] [diff] [review]
patch 3
Attachment #631198 - Flags: review?(roc)
(Assignee)

Comment 30

5 years ago
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
containing:
https://hg.mozilla.org/mozilla-central/rev/58bc22e53542
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/be9f4bd44b64/update-width-cause-reflow
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/be9f4bd44b64/dirty-intrinsic-widths
(Assignee)

Updated

5 years ago
Blocks: 762372, 757179
Attachment #631198 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Attachment #630776 - Attachment description: patch → patch 2
(Assignee)

Updated

5 years ago
Attachment #631198 - Attachment description: patch, part 2 → patch 3
(Assignee)

Comment 31

5 years ago
Created attachment 631228 [details] [diff] [review]
patch 2

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)
(Assignee)

Comment 32

5 years ago
Created attachment 631229 [details] [diff] [review]
patch 4: make svg:foreignObject considered a constrained height

And while I have svg:foreignObject in my head, they should be considered a constrained height.
Attachment #631229 - Flags: review?(roc)
Attachment #631228 - Flags: review?(roc) → review+
Attachment #631229 - Flags: review?(roc) → review+
(Assignee)

Comment 33

5 years ago
(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
(Assignee)

Comment 34

5 years ago
Test build and try run with current set of patches:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-958f58b3dd2a/try-android/fennec-16.0a1.en-US.android-arm.apk
https://tbpl.mozilla.org/?tree=Try&rev=958f58b3dd2a
(Assignee)

Comment 35

5 years ago
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.
(Assignee)

Comment 36

5 years ago
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
(Assignee)

Comment 37

5 years ago
(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.
(Assignee)

Comment 38

5 years ago
https://hg.mozilla.org/mozilla-central/rev/470130288d35
https://hg.mozilla.org/mozilla-central/rev/753567e35350
https://hg.mozilla.org/mozilla-central/rev/876d3ce8a630
https://hg.mozilla.org/mozilla-central/rev/679e4b464c10
(Assignee)

Comment 39

5 years ago
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).
(Assignee)

Comment 40

5 years ago
Created attachment 631801 [details] [diff] [review]
patch 2

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)
(Assignee)

Comment 41

5 years ago
(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
(Assignee)

Comment 45

5 years ago
Created attachment 632005 [details] [diff] [review]
interdiff against previous reviewed patch 2
(Assignee)

Updated

5 years ago
Attachment #632005 - Flags: review?(bzbarsky)
Comment on attachment 632005 [details] [diff] [review]
interdiff against previous reviewed patch 2

r=me
Attachment #632005 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 47

5 years ago
https://hg.mozilla.org/mozilla-central/rev/649beef6950c
https://hg.mozilla.org/mozilla-central/rev/b7dd74f5a7d2
(Assignee)

Updated

5 years ago
Attachment #631801 - Flags: review?(roc)
(Assignee)

Comment 48

5 years ago
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
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.
Created attachment 632060 [details]
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)
Created attachment 632062 [details]
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.
(Assignee)

Comment 53

5 years ago
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.
(Assignee)

Comment 55

5 years ago
An experiment (mainly for myself):
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-76b9ad1e5c8e/try-android/fennec-16.0a1.en-US.android-arm.apk
(Assignee)

Comment 56

5 years ago
Actually, that seems to fix the problem.
(Assignee)

Comment 57

5 years ago
Created attachment 632110 [details] [diff] [review]
patch 5

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)
(Assignee)

Comment 58

5 years ago
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
(Assignee)

Comment 61

5 years ago
(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.
(Assignee)

Comment 62

5 years ago
https://hg.mozilla.org/mozilla-central/rev/131961e5e0d1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Assignee)

Comment 63

5 years ago
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?
(Assignee)

Comment 64

5 years ago
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?
(Assignee)

Comment 65

5 years ago
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?
(Assignee)

Comment 66

5 years ago
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+
(Assignee)

Comment 67

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ae9adf800cf
https://hg.mozilla.org/releases/mozilla-aurora/rev/79fee919312e
https://hg.mozilla.org/releases/mozilla-aurora/rev/87d73102b9c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcd1e63c8112
https://hg.mozilla.org/releases/mozilla-aurora/rev/201c182ffd35

https://hg.mozilla.org/releases/mozilla-beta/rev/ddb6757e3b9b
https://hg.mozilla.org/releases/mozilla-beta/rev/cee1487b27e3
https://hg.mozilla.org/releases/mozilla-beta/rev/c07ddb843d31
https://hg.mozilla.org/releases/mozilla-beta/rev/bac478b60979
https://hg.mozilla.org/releases/mozilla-beta/rev/ed6a02ba84f0
status-firefox14: --- → fixed
status-firefox15: --- → fixed

Updated

5 years ago
Depends on: 764354
Depends on: 764510

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762372
You need to log in before you can comment on or make changes to this bug.