Closed
Bug 1138356
Opened 10 years ago
Closed 10 years ago
button contents not properly aligned in vertical-rl mode when button width varies
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
2.42 KB,
text/html
|
Details | |
5.31 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
7.58 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8571265 -
Flags: review?(smontagu)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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?
Attachment #8571265 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8571265 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8576643 -
Flags: review?(smontagu)
Assignee | ||
Updated•10 years ago
|
Attachment #8575387 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
And let's have a reftest to go with it. This fails on current trunk (both tests), and passes with the patch.
Attachment #8576645 -
Flags: review?(smontagu)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Just a minor update to the reftests to make them work better across platforms with different font metrics.
Attachment #8576857 -
Flags: review?(smontagu)
Assignee | ||
Updated•10 years ago
|
Attachment #8576645 -
Attachment is obsolete: true
Attachment #8576645 -
Flags: review?(smontagu)
Updated•10 years ago
|
Attachment #8576643 -
Flags: review?(smontagu) → review+
Updated•10 years ago
|
Attachment #8576857 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70ca84cce157
https://hg.mozilla.org/mozilla-central/rev/6cb698642997
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•