Closed
Bug 737621
Opened 13 years ago
Closed 13 years ago
link wording temporarily grow smaller if you click on a link to go to a page
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla15
People
(Reporter: nhirata, Assigned: dbaron)
References
()
Details
(Keywords: mobile, reproducible, Whiteboard: readability, testcase)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jwir3
:
review+
blassey
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
I don't see it with that page, but I see it when tapping on the first link in bug 737529, comment 0.
Blocks: font-inflation
Comment 2•13 years ago
|
||
Hmm, I'm also seeing this in about:crashes. I think this might be a recent regression, not sure.
Comment 3•13 years ago
|
||
I'm also seeing it with this rather simple page: http://codinginparadise.org/projects/svgweb/samples/
Comment 4•13 years ago
|
||
Updated•13 years ago
|
Attachment #610535 -
Attachment is obsolete: true
Comment 7•13 years ago
|
||
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
Updated•13 years ago
|
blocking-fennec1.0: --- → ?
Comment 8•13 years ago
|
||
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: ? → +
Comment 9•13 years ago
|
||
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 11•13 years ago
|
||
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?
Updated•13 years ago
|
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Version: Firefox 14 → Trunk
Updated•13 years ago
|
Keywords: reproducible
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
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).
Assignee | ||
Comment 19•13 years ago
|
||
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.
Assignee | ||
Comment 20•13 years ago
|
||
(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|).
Assignee | ||
Comment 21•13 years ago
|
||
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.
Assignee | ||
Comment 22•13 years ago
|
||
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.
Assignee | ||
Comment 24•13 years ago
|
||
(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.
Assignee | ||
Comment 25•13 years ago
|
||
Aha, nsContentUtils::GetViewportInfo is returning what it's returning because aDocument->GetWindow() is null.
Assignee | ||
Comment 26•13 years ago
|
||
... 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.
Assignee | ||
Comment 27•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 29•13 years ago
|
||
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 :-)
Assignee | ||
Comment 30•13 years ago
|
||
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.
Assignee | ||
Comment 31•13 years ago
|
||
(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.
Assignee | ||
Comment 32•13 years ago
|
||
I considered landing this, but didn't because the tree was a mess due to bug 748939.
Assignee | ||
Comment 33•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/add8917f9123
Target Milestone: --- → mozilla15
Assignee | ||
Comment 34•13 years ago
|
||
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?
Comment 35•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/add8917f9123
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #618386 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Updated•13 years ago
|
status-firefox14:
--- → fixed
Comment 37•13 years ago
|
||
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.
Description
•