Closed Bug 983691 Opened 6 years ago Closed 6 years ago

Facebook post buttons have icons and labels misaligned

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 + fixed
firefox31 --- fixed

People

(Reporter: Logan, Assigned: smontagu)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image screenshot of issue
This is definitely a regression. I'll find a window shortly.
Screenshot attached.
This is the best I could do in terms of a regression window with mozregression:

Last good revision: 44ae8462d6ab (2014-03-12)
First bad revision: 46041cc216fd (2014-03-13)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd

I tried bisecting inbounds, but all of them appeared to be bad (after two runs), and it said that there were no left to run, and it was just this commit left, which would make no sense: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=44ae8462d6ab&tochange=22d46873261d
It's possible.  Nothing else in that range looks terribly likely either....

Is there a way to reproduce this bug without having a facebook account?
Flags: needinfo?(jdemooij)
This looks like the most likely culprit in that range:
https://hg.mozilla.org/mozilla-central/rev/dab8e3865967
Duplicate of this bug: 983872
Not bug 982186, I can also reproduce it with an older inbound revision.

(In reply to Mats Palmgren (:mats) from comment #3)
> This looks like the most likely culprit in that range:
> https://hg.mozilla.org/mozilla-central/rev/dab8e3865967

Yes, I confirmed that's the regressor. Inbound rev f05bca38aa4d works, the next revision is dab8e3865967 and doesn't work.
Blocks: 789096
Component: Layout → Layout: Text
Flags: needinfo?(jdemooij) → needinfo?(smontagu)
(In reply to Boris Zbarsky [:bz] from comment #2)
> Is there a way to reproduce this bug without having a facebook account?

Open this page: https://www.facebook.com/BillGates and scroll down to posts around February 5 - 10, you should see the problem there.
Attached file Minimized testcase
Assignee: nobody → smontagu
Flags: needinfo?(smontagu)
Comment on attachment 8391775 [details] [diff] [review]
983691.diff

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

Oops! Yes, this looks correct.
Attachment #8391775 - Flags: review?(jfkthame) → review+
Do you understand why the reftest failed on the Windows Try builds? At a pinch I could just mark it as failing on Windows and rely on the OSX and Linux coverage, but I don't know any reason why it shouldn't work there and I'm concerned that there's some other issue here.
Flags: needinfo?(jfkthame)
Ugh - sorry, I didn't see those results last night.

I suspect this may be due to the variations in how we handle font line-spacing metrics across platforms; and as such, it suggests this test is going to be too fragile. :( We have a couple of open bugs that will adjust how we set up the metrics, and it wouldn't surprise me if that alters things here.

I pushed a tryserver job with this testcase on top of f05bca38aa4d, the changeset *before* this regression (see comment 5), and it confirms that the same failure happens there. (And similarly on Android, FWIW): https://tbpl.mozilla.org/?tree=Try&rev=0d5bf05e25bf.

So I'm inclined to think the failure of the new test on some platforms is a pre-existing issue, probably related to the different font back-ends, but it does suggest we should try to come up with a less fragile version of the test for this bug. The actual code patch here is still correct, though.

Your try push also shows a (Windows D2D-only) failure on text-subpixel-1.html, so we should check that as well - offhand (without having looked at it), I don't see an obvious reason it should have been affected.
Flags: needinfo?(jfkthame)
Duplicate of this bug: 984053
(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Your try push also shows a (Windows D2D-only) failure on
> text-subpixel-1.html, so we should check that as well - offhand (without
> having looked at it), I don't see an obvious reason it should have been
> affected.

Never mind about that; looks like it was bug 982480, which was landed at the time of your try run, but has bounced due to this and other issues.
I'd guess that using 'line-height: 1.0' explicitly might help with the reftest -- 'line-height: normal' not being equivalent to 1.0 would explain the failure.
Also, there are tabs on two lines of the test file.
Attached patch 983691.diffSplinter Review
With thanks to dbaron and jdaggett: the reftest seems to be more stable on all platforms if I (a) specify line-height: 1.0; (b) use a less generic name for the downloaded font; (c) use a large multiple of 5 for font-size. Black magic, but effective!

https://hg.mozilla.org/try/rev/4e57195deda8
Attachment #8391775 - Attachment is obsolete: true
Attachment #8393960 - Flags: review?(jfkthame)
Comment on attachment 8393960 [details] [diff] [review]
983691.diff

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

I still wonder whether the reftest will be sensitive to tweaks in font-metrics code, but I guess we'll have to deal with that if/when it arises. (It may prove helpful in highlighting unexpected platform differences that previous tests have missed, anyhow.)
Attachment #8393960 - Flags: review?(jfkthame) → review+
FWIW, I think any multiple of 5 will do; I don't think it needs to be a large one.  (I'd forgotten about that bit.)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
This needs to get onto aurora as well.
Flags: needinfo?(smontagu)
Comment on attachment 8393960 [details] [diff] [review]
983691.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789096
User impact if declined: misalignment of elements using 'vertical-align: text-bottom', including buttons on facebook
Testing completed (on m-c, etc.): on mozilla-central for a bit now; has reftest
Risk to taking this patch (and alternatives if risky): low risk; restores previous behavior
String or IDL/UUID changes made by this patch: no
Attachment #8393960 - Flags: approval-mozilla-aurora?
Attachment #8393960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(smontagu)
You need to log in before you can comment on or make changes to this bug.