Last Comment Bug 759755 - Links on some web pages are rendered misplaced
: Links on some web pages are rendered misplaced
Status: VERIFIED FIXED
[readability]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
:
Mentors:
: 762372 (view as bug list)
Depends on: 763434 764354 764497 764510
Blocks: 757179 762372
  Show dependency treegraph
 
Reported: 2012-05-30 07:55 PDT by :Ehsan Akhgari (away Aug 1-5)
Modified: 2012-07-24 18:37 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
+


Attachments
screenshot (121.02 KB, image/png)
2012-05-30 14:12 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details
patch 2 (4.86 KB, patch)
2012-06-06 17:14 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
roc: review+
Details | Diff | Splinter Review
patch 3 (4.57 KB, patch)
2012-06-07 16:06 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch 2 (7.70 KB, patch)
2012-06-07 17:34 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
roc: review+
Details | Diff | Splinter Review
patch 4: make svg:foreignObject considered a constrained height (1.33 KB, patch)
2012-06-07 17:35 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch 2 (7.97 KB, patch)
2012-06-10 21:20 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review
interdiff against previous reviewed patch 2 (3.63 KB, patch)
2012-06-11 13:24 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
Details | Diff | Splinter Review
Screenshot (59.55 KB, image/png)
2012-06-11 16:26 PDT, Naoki Hirata :nhirata (please use needinfo instead of cc)
no flags Details
Screenshot 2 (87.60 KB, image/png)
2012-06-11 16:29 PDT, Naoki Hirata :nhirata (please use needinfo instead of cc)
no flags Details
patch 5 (2.58 KB, patch)
2012-06-11 19:33 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Ehsan Akhgari (away Aug 1-5) 2012-05-30 07:55:41 PDT
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...
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-30 12:36:12 PDT
Ehsan can you try with the font size set to 0?  (Font inflation turned off)
Comment 2 Joe Drew (not getting mail) 2012-05-30 12:37:38 PDT
Please to screenshot, sir.
Comment 3 :Ehsan Akhgari (away Aug 1-5) 2012-05-30 14:06:50 PDT
(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?
Comment 4 :Ehsan Akhgari (away Aug 1-5) 2012-05-30 14:12:59 PDT
Created attachment 628475 [details]
screenshot
Comment 5 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-30 15:10:21 PDT
(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.
Comment 6 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-30 15:16:37 PDT
Could you also place which build you have installed please?
Comment 7 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-30 15:27:27 PDT
I do not see this issue on the Samsung Galaxy S Captivate w/ today's nightly build 5/30/2012
Comment 8 :Ehsan Akhgari (away Aug 1-5) 2012-05-30 15:52:39 PDT
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.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-31 12:01:55 PDT
This looks OK for me on Nightly. Can you try it on Nightly?
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-01 14:43:21 PDT
I just saw this on today's nightly.
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-01 14:46:30 PDT
I think the conditions for reproducing relate to text run cache expiration.
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-01 14:47:45 PDT
likely regressio from bug 747720, I think
Comment 13 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-04 13:23:03 PDT
maybe related to bug 757179
Comment 14 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-04 18:15:34 PDT
(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 Jet Villegas (:jet) 2012-06-06 12:27:04 PDT
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?
Comment 16 Scott Johnson (:jwir3) 2012-06-06 14:56:42 PDT
(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?
Comment 17 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-06 16:32:23 PDT
(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).
Comment 18 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-06 17:14:44 PDT
Created attachment 630776 [details] [diff] [review]
patch 2

I think this should fix it, though I haven't yet tested it.
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-06 20:26:14 PDT
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 JP Rosevear [:jpr] 2012-06-06 21:33:44 PDT
Please test build in #19
Comment 21 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-06 22:05:34 PDT
(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.)
Comment 22 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-06 22:13:16 PDT
Landed anyway:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e441f7ad9d
Comment 24 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 14:08:19 PDT
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...)
Comment 25 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 14:17:27 PDT
... though I'd have thought that problem would have been fixed by bug 747231.
Comment 26 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 14:30:33 PDT
This patch does fix bug 757179 in that it makes the display consistent, though we end up being consistent on the less preferred behavior.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 14:33:55 PDT
Er, maybe it doesn't, actually.
Comment 28 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 14:37:12 PDT
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).
Comment 29 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 16:06:33 PDT
Created attachment 631198 [details] [diff] [review]
patch 3
Comment 31 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 17:34:59 PDT
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.
Comment 32 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 17:35:50 PDT
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.
Comment 33 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 17:57:27 PDT
(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
Comment 34 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-07 21:41:16 PDT
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
Comment 35 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-08 12:13:37 PDT
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.
Comment 36 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-08 14:45:43 PDT
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
Comment 37 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-08 14:56:32 PDT
(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.
Comment 39 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-10 17:39:12 PDT
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).
Comment 40 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-10 21:20:31 PDT
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.)
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-10 21:21:22 PDT
(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.
Comment 42 Ed Morley [:emorley] 2012-06-11 04:22:19 PDT
(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?
Comment 43 Aaron Train [:aaronmt] 2012-06-11 09:32:14 PDT
Re comment #19; do we have a newer build here for testing?
Comment 44 Ed Morley [:emorley] 2012-06-11 11:57:30 PDT
(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 45 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 13:24:38 PDT
Created attachment 632005 [details] [diff] [review]
interdiff against previous reviewed patch 2
Comment 46 Boris Zbarsky [:bz] 2012-06-11 13:54:06 PDT
Comment on attachment 632005 [details] [diff] [review]
interdiff against previous reviewed patch 2

r=me
Comment 47 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 13:59:31 PDT
https://hg.mozilla.org/mozilla-central/rev/649beef6950c
https://hg.mozilla.org/mozilla-central/rev/b7dd74f5a7d2
Comment 48 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 15:24:40 PDT
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
Comment 49 Tony Chung [:tchung] 2012-06-11 15:56:18 PDT
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.
Comment 50 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-11 16:23:01 PDT
AFAIK, we're trying to figure this out before 5 PM PDT.
Testing around : bug 747231 would be a good idea as well.
Comment 51 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-11 16:26:43 PDT
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)
Comment 52 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-11 16:29:33 PDT
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.
Comment 53 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 16:38:15 PDT
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.
Comment 54 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-11 16:53:58 PDT
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.
Comment 55 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 19:18:54 PDT
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
Comment 56 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 19:22:59 PDT
Actually, that seems to fix the problem.
Comment 57 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 19:33:24 PDT
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).
Comment 58 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 19:45:30 PDT
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 59 Boris Zbarsky [:bz] 2012-06-11 19:52:45 PDT
Comment on attachment 632110 [details] [diff] [review]
patch 5

s/calld/called/ and r=me
Comment 60 Tony Chung [:tchung] 2012-06-11 19:58:38 PDT
(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
Comment 61 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 22:38:12 PDT
(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.
Comment 62 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 22:45:23 PDT
https://hg.mozilla.org/mozilla-central/rev/131961e5e0d1
Comment 63 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 22:57:39 PDT
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
Comment 64 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 23:02:17 PDT
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
Comment 65 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 23:04:47 PDT
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
Comment 66 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-11 23:09:04 PDT
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
Comment 68 Tony Chung [:tchung] 2012-06-18 09:22:52 PDT
kevin: please take a look to see if its fixed, thanks
Comment 69 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-18 16:57:25 PDT
This seems fixed on the Galaxy S II using Nightly 06/18/2012 build.  Leaving for review from kbrosnan.
Comment 70 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-25 16:18:45 PDT
Removing QA wanted and placing verified from comment 69, and tomorrow being release.
Comment 71 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-07-24 18:37:19 PDT
*** Bug 762372 has been marked as a duplicate of this bug. ***

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