Last Comment Bug 737621 - link wording temporarily grow smaller if you click on a link to go to a page
: link wording temporarily grow smaller if you click on a link to go to a page
Status: VERIFIED FIXED
readability, testcase
: mobile, reproducible
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
:
Mentors:
http://bit.ly/IPCs5D
: 741419 745242 (view as bug list)
Depends on:
Blocks: font-inflation 706198
  Show dependency treegraph
 
Reported: 2012-03-20 13:40 PDT by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2012-05-28 05:05 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+


Attachments
testcase (511 bytes, text/html)
2012-03-29 07:13 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (497 bytes, text/html)
2012-03-29 07:14 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (613 bytes, text/html)
2012-03-29 07:17 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (1.60 KB, patch)
2012-04-25 12:19 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
jaywir3: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-03-20 13:40:53 PDT
1. go to http://people.mozilla.com/~nhirata/html_tp/
2. click on a link

Expected: lettering stays the same size before going to link
Actual: lettering goes smaller before going to link

Note:
Galaxy Nexus , 3/20/2012 Nightly
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-23 07:36:49 PDT
I don't see it with that page, but I see it when tapping on the first link in bug 737529, comment 0.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-23 08:03:43 PDT
Hmm, I'm also seeing this in about:crashes. I think this might be a recent regression, not sure.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-29 05:20:25 PDT
I'm also seeing it with this rather simple page: http://codinginparadise.org/projects/svgweb/samples/
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-29 07:13:34 PDT
Created attachment 610532 [details]
testcase
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-29 07:14:41 PDT
Created attachment 610534 [details]
testcase
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-29 07:17:49 PDT
Created attachment 610535 [details]
testcase
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-29 07:21:10 PDT
Sorry, I'm unable to attach a testcase to this bug, because I need to change the '?' part of the url. I have a testcase here:
http://people.mozilla.org/~mwargers/tests/layout/fontinflation_linkshrink.htm
You should see the 2nd link shrinking after every 2 seconds if this bug occurs.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-11 12:35:56 PDT
Is this related to font-inflation? Can we try to disable font-inflation and test? If it's font-inflation, we should assign to someone else.
Comment 9 Lucas Rocha (:lucasr) 2012-04-12 09:38:21 PDT
When I disable font inflation, fonts keep the same size when clicked. So, I assume this is font inflation bug. Who should be the assignee then?
Comment 10 Scott Johnson (:jwir3) 2012-04-12 13:21:10 PDT
I'll take a look at it, Lucas.
Comment 11 Scott Johnson (:jwir3) 2012-04-12 13:27:48 PDT
What I'm seeing is that the links in the example in comment 7 are getting smaller for a half second or so when the page is refreshing, but then get larger again after the page completes its refresh... is this consistent with what you're expecting that I see?
Comment 12 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-12 13:32:58 PDT
Yes, Scott.
Comment 13 Kevin Brosnan [:kbrosnan] 2012-04-13 14:11:56 PDT
*** Bug 745242 has been marked as a duplicate of this bug. ***
Comment 14 Scott Johnson (:jwir3) 2012-04-13 15:03:53 PDT
Shortened URL for testcase.
Comment 15 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-19 15:16:59 PDT
*** Bug 741419 has been marked as a duplicate of this bug. ***
Comment 16 Aaron Train [:aaronmt] 2012-04-19 15:56:52 PDT
Ah yes, I see this tapping links on about:about
Comment 17 Scott Johnson (:jwir3) 2012-04-24 13:13:48 PDT
dbaron and I spoke about this last week, and we think this might have something to do with the focus outline for links. This is the only thing that would cause reflow for a frame when navigating through a link.

I'm looking into this at the moment, and I hope to have an answer as to why this is happening. Bug 746966 is taking precedence, though.
Comment 18 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 10:28:29 PDT
I just tried debugging this briefly, and couldn't actually find anything showing where the smaller text might be coming from.  In particular, I checked that:

 * in every call to nsTextFrame::ReflowText, for the frames in the page (rather than the refresh of the URL bar), |fontSizeInflation| is 4.2... or 4.3... (maybe the variation is reflows related to the scrollbar hypothetical, I hope?).

 * in every call to nsTextFrame::SetTextRun for the frames in the page, aInflation is 4.something, and aWhichTextRun is nsTextFrame::eInflated.

I've been doing:

b nsTextFrame::ReflowText
commands
  p fontSizeInflation
  p mContent->mText.m1b
end

b nsTextFrame::SetTextRun
commands
  p aInflation
  p aWhichTextRun
  p mContent->mText.m1b
end

while showing the URL field of this bug.

Though I'm not sure if I'm seeing the text flash smaller either (though I definitely was earlier).
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 11:24:32 PDT
So I think I only see the problem on desktop if I at some point open the new tab page *after* having the testcase open.
Comment 20 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 11:27:45 PDT
(In reply to David Baron [:dbaron] from comment #18)
> b nsTextFrame::ReflowText

er, I meant "b nsTextFrameThebes.cpp:7401" (the line with the use of |fontSizeInflation|).
Comment 21 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 11:37:51 PDT
So the one time so far I caught the problem in the debugger, the bad inflation number was being set during text run construction that was happening during paint.  So I suspect that the issue may (a) involve racing against the text run discarding timer and (b) be a problem with the not-in-reflow width computation.
Comment 22 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 11:50:04 PDT
Got a few steps earlier in the debugger: inside the text run construction that happens inside PaintText, the problem actually seems to be that nsLayoutUtils::FontSizeInflationEnabled is returning false because nsContentUtils::GetViewportInfo() returned a value such that vInf.autoSize is true.
Comment 23 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 11:55:20 PDT
but then the second time through that's not what I'm seeing
Comment 24 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 12:03:34 PDT
(In reply to David Baron [:dbaron] from comment #23)
> but then the second time through that's not what I'm seeing

Of course, the second time through I get an inflation factor of 4.31..., which is correct.
Comment 25 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 12:08:21 PDT
Aha, nsContentUtils::GetViewportInfo is returning what it's returning because aDocument->GetWindow() is null.
Comment 26 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 12:12:47 PDT
... and I think that entire variable and null-check could have just been removed between patches v5 and v6 of bug 706198, so I think the best thing is to just remove them now.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 12:19:20 PDT
Created attachment 618386 [details] [diff] [review]
patch

So when I follow the steps that led me to see the bug before (load URL in testcase in a desktop build with emPerLine set to 12 and lineThreshold set to 0, then open new tab, then switch back) this does appear to fix the bug (and I see the link repainting as purple (visited) very briefly, at the point when without the fix it would have resized).
Comment 28 Scott Johnson (:jwir3) 2012-04-25 13:09:04 PDT
Comment on attachment 618386 [details] [diff] [review]
patch

Review of attachment 618386 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentUtils.cpp
@@ -4853,5 @@
> -
> -  if (!windowUtils) {
> -    return ret;
> -  }
> -

Hm, yes, I do see the comment you made in bug 706198 about this. I apologize for accidentally adding this back into the code between patches v5 and v6, when it should have been removed. It was certainly not an intentional disregard of your review comments, but I apologize for having to have you waste your time debugging this issue nonetheless.

r=sjohnson
Comment 29 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 13:17:54 PDT
No need to apologize -- you made the changes that were the main point of the comments.  As you write more Mozilla code you're going to have to get used to other people finding bugs in code you wrote, though :-)
Comment 30 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 13:20:23 PDT
Also, to be explicit about one other thing:  I think it's probably not worth trying to write an automated test for this.  While it might be possible, I think it would take a good bit of effort to write a test that shows the bug at all, and even then there'd be little guarantee that such a test would continue to exercise this code after future changes.
Comment 31 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 13:22:07 PDT
(In reply to David Baron [:dbaron] from comment #29)
> No need to apologize -- you made the changes that were the main point of the
> comments.  As you write more Mozilla code you're going to have to get used
> to other people finding bugs in code you wrote, though :-)

Oh... and since I noticed it's possible to read this wrong, I should say that by this I mean that everybody who writes any substantive amount of code is going to write bugs.
Comment 32 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 14:16:07 PDT
I considered landing this, but didn't because the tree was a mess due to bug 748939.
Comment 33 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 16:37:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/add8917f9123
Comment 34 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-25 16:40:55 PDT
Comment on attachment 618386 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): bug 706198
User impact if declined: Font inflation gets turned off during repaints/reflows that happen while we're in the process of navigating away from a page, which most commonly happens when the user clicks on a link and we repaint that link
Testing completed (on m-c, etc.): will be on mozilla-central tomorrow (i.e., hopefully by the time you read this)
Risk to taking this patch (and alternatives if risky): low -- only possible concern is the possibility that one of the later functions called in the function being modified might have a worse failure mode when the document is being navigated away from (and the window is connected to the new document)
String changes made by this patch: none
Comment 35 Ed Morley [:emorley] 2012-04-26 10:44:10 PDT
https://hg.mozilla.org/mozilla-central/rev/add8917f9123
Comment 36 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-26 13:46:46 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1efdb471821
Comment 37 Andreea Pod 2012-05-28 05:05:55 PDT
I went to this test page: http://people.mozilla.org/~mwargers/tests/layout/fontinflation_linkshrink.htm from comment 7. The second link is not shrinking.

Verified fixed on: Firefox 15.0a1 (2012-05-27)
Device: LG Optimus 2X (Android 2.2.2)

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