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

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: nhirata, Assigned: dbaron)

Tracking

({mobile, reproducible})

Trunk
mozilla15
ARM
Android
mobile, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 +)

Details

(Whiteboard: readability, testcase, URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
Blocks: 627842
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/
Created attachment 610532 [details]
testcase
Created attachment 610534 [details]
testcase
Attachment #610532 - Attachment is obsolete: true
Created attachment 610535 [details]
testcase
Attachment #610534 - Attachment is obsolete: true

Updated

5 years ago
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

Updated

5 years ago
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?
Yes, Scott.
Duplicate of this bug: 745242
Shortened URL for testcase.

Updated

5 years ago
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Version: Firefox 14 → Trunk

Updated

5 years ago
Keywords: mobile
Duplicate of this bug: 741419
Ah yes, I see this tapping links on about:about

Updated

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

Comment 18

5 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

5 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

5 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

5 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

5 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 23

5 years ago
but then the second time through that's not what I'm seeing
(Assignee)

Comment 24

5 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

5 years ago
Aha, nsContentUtils::GetViewportInfo is returning what it's returning because aDocument->GetWindow() is null.
(Assignee)

Comment 26

5 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

5 years ago
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).
Assignee: sjohnson → dbaron
Status: NEW → ASSIGNED
Attachment #618386 - Flags: review?(sjohnson)
(Assignee)

Updated

5 years ago
Blocks: 706198
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

5 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

5 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

5 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

5 years ago
I considered landing this, but didn't because the tree was a mess due to bug 748939.
(Assignee)

Comment 33

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/add8917f9123
Target Milestone: --- → mozilla15
(Assignee)

Comment 34

5 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?
https://hg.mozilla.org/mozilla-central/rev/add8917f9123
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #618386 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 36

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1efdb471821
(Assignee)

Updated

5 years ago
status-firefox14: --- → fixed

Comment 37

5 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.