Closed Bug 737621 Opened 12 years ago Closed 12 years ago

link wording temporarily grow smaller if you click on a link to go to a page

Categories

(Core :: Layout, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- +

People

(Reporter: nhirata, Assigned: dbaron)

References

()

Details

(Keywords: mobile, reproducible, Whiteboard: readability, testcase)

Attachments

(1 file, 3 obsolete files)

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
I don't see it with that page, but I see it when tapping on the first link in bug 737529, comment 0.
Hmm, I'm also seeing this in about:crashes. I think this might be a recent regression, not sure.
I'm also seeing it with this rather simple page: http://codinginparadise.org/projects/svgweb/samples/
Attached file testcase (obsolete) —
Attached file testcase (obsolete) —
Attachment #610532 - Attachment is obsolete: true
Attached file testcase (obsolete) —
Attachment #610534 - Attachment is obsolete: true
Attachment #610535 - Attachment is obsolete: true
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.
Whiteboard: readability → readability, testcase
blocking-fennec1.0: --- → ?
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.
Assignee: nobody → lucasr.at.mozilla
blocking-fennec1.0: ? → +
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?
I'll take a look at it, Lucas.
Assignee: lucasr.at.mozilla → sjohnson
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?
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Version: Firefox 14 → Trunk
Keywords: mobile
Ah yes, I see this tapping links on about:about
Keywords: reproducible
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.
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).
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.
(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|).
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.
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.
but then the second time through that's not what I'm seeing
(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.
Aha, nsContentUtils::GetViewportInfo is returning what it's returning because aDocument->GetWindow() is null.
... 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.
Attached patch patchSplinter Review
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).
Assignee: sjohnson → dbaron
Status: NEW → ASSIGNED
Attachment #618386 - Flags: review?(sjohnson)
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
Attachment #618386 - Flags: review?(sjohnson) → review+
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 :-)
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.
(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.
I considered landing this, but didn't because the tree was a mess due to bug 748939.
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
Attachment #618386 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/add8917f9123
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #618386 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: