1138356 - button contents not properly aligned in vertical-rl mode when button width varies

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase with vertical-mode buttons of various widths

See testcase; as the width (block-size) of the vertical-rl buttons increases, the content is offset by an increasing amount in the wrong direction.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Correct the alignment of button contents in vertical-rl writing mode

Comment 2

[Simon Montagu :smontagu]

Comment on attachment 8571265 [details] [diff] [review]
Correct the alignment of button contents in vertical-rl writing mode

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

::: layout/forms/nsHTMLButtonControlFrame.cpp
@@ +362,1 @@
>  

Can you explain (or comment) why the condition is IsVerticalRL() in one place and IsLineInverted() in the other?

Comment 3

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e09a396d677

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #2)
> Can you explain (or comment) why the condition is IsVerticalRL() in one
> place and IsLineInverted() in the other?

Better than that :) .... I can remove the second change altogether, as re-testing with asymmetrical padding convinces me that it's the wrong thing to do anyway.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Correct the alignment of button contents in vertical-rl writing mode

Tidied-up version, carrying over r=smontagu from above. I've pushed a try job (comment 3) to double-check that it still behaves as expected.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Correct the alignment of button contents in vertical-rl writing mode

OK, I finally decided it was impossible to add a comment justifying the IsVerticalRL() test and the negation of extraSpace, because that's simply wrong. It was basically compensating for the use of an incorrect containerWidth when calling FinishReflowChild, but two wrongs don't make a right. So I revisited this and have finally fixed it correctly, I believe. Marking for re-review as by now this is a completely different patch.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for button contents alignment in vertical writing modes

And let's have a reftest to go with it. This fails on current trunk (both tests), and passes with the patch.

Comment 8

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b66318dfd0f

Comment 9

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7082a84304

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for button contents alignment in vertical writing modes

Just a minor update to the reftests to make them work better across platforms with different font metrics.

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ca84cce157
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb698642997

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70ca84cce157
https://hg.mozilla.org/mozilla-central/rev/6cb698642997