Closed Bug 1130231 Opened 10 years ago Closed 10 years ago

[RTL] padding-left/right reverses on buttons with border

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 + fixed
firefox38 + verified
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mikehenrty, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(3 files)

Reftest for left/right padding on button contents with direction=rtl
(deleted), patch
smontagu
: review+
Details | Diff | Splinter Review
Properly logicalize the implementation of padding on button contents
(deleted), patch
smontagu
: review+
lmandel
: approval-mozilla-aurora+
Details | Diff | Splinter Review
[Aurora] Properly logicalize the implementation of padding on button contents
(deleted), patch
smontagu
: review+
Details | Diff | Splinter Review
Padding-left will apply padding to the right side of a button element, and vice versa, when document is in RTL mode. Note, this padding only gets applied when the button element has a border for some reason. Also, this has been reproduced on both Firefox OS, and on OSX Yosemite. Below is a minimum case that repros: <!doctype html> <html lang="en" dir="rtl"> <head> <meta charset="utf-8"> <title>blah</title> <style type="text/css"> .inner { padding-left: 100px; border: 1px solid black; } </style> </head> <html> <body> <button class="inner">**aligned**</button> </body> </html>
Also note that using `-moz-padding-start: 100px` will always apply to the left side of the element, no matter how the document dir is set.
Simon, is this actually a bug? This effects offline error pages in b2g in RTL mode (bug 1119398), which is blocking 2.2. Knowing if this is a bug, and the effort required to fix will help us decide how to proceed there.
Flags: needinfo?(smontagu)
Seems likely to be a regression from https://hg.mozilla.org/mozilla-central/rev/9c33d0beeccf which converted the call in nsHTMLButtonControlFrame::ReflowButtonContents to FinishReflowChild to use logical coordinates without switching the function being called to the logical-coordinate version of FinishReflowChild.
Blocks: 1106667
(It might be worth double-checking that the regression range matches that; I didn't check.)
(In reply to Michael Henretty [:mhenretty] from comment #2) > Simon, is this actually a bug? This effects offline error pages in b2g in > RTL mode (bug 1119398), which is blocking 2.2. Knowing if this is a bug, and > the effort required to fix will help us decide how to proceed there. Definitely a bug, and we'll fix it pronto; you should not need to hack around this.
Flags: needinfo?(smontagu)
Here are a couple of reftests based on the example here. These both fail with current trunk code (though they pass on older versions), confirming this is a regression.
Attachment #8560387 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
And here's a patch to fix this, by switching to the logical child methods.
Attachment #8560388 - Flags: review?(smontagu)
[Tracking Requested - why for this release]: This is a recent regression that will affect HTML buttons (depending on the platform and styling -- platform-native buttons appear to be unaffected) in right-to-left languages.
status-firefox36: --- → unaffected
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox37: --- → ?
Tracking this regression.
status-firefox35: --- → unaffected
tracking-firefox37: ? → +
tracking-firefox38: --- → +
Keywords: regression
Attachment #8560387 - Flags: review?(smontagu) → review+
Comment on attachment 8560388 [details] [diff] [review] Properly logicalize the implementation of padding on button contents Review of attachment 8560388 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8560388 - Flags: review?(smontagu) → review+
Comment on attachment 8560388 [details] [diff] [review] Properly logicalize the implementation of padding on button contents Approval Request Comment [Feature/regressing bug #]: 1106667 [User impact if declined]: incorrect placement of button contents in RTL when padding is applied -- in particular, affects some B2G screens [Describe test coverage new/current, TreeHerder]: reftests for both physical and logical padding landed in this bug [Risks and why]: low risk - affects nothing outside button contents [String/UUID change made/needed]: none
Attachment #8560388 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/440781710521 https://hg.mozilla.org/mozilla-central/rev/8f675794e1b6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Hello Jonathan, and Micheal. I was just wondering if there was any way I could get some manual steps to verify this fix? Thanks.
Flags: needinfo?(jfkthame)
Flags: needinfo?(mhenretty)
(In reply to Derek Harris [:DerekH] from comment #14) > Hello Jonathan, and Micheal. I was just wondering if there was any way I > could get some manual steps to verify this fix? Thanks. Copy the HTML from comment 0 into a web an .html file and open that with a build of the browser containing this fix. A working version should contain the text '**aligned**' on the right side of the button with a lot of space on the left. A broken version will show the text on the left and space on the right.
Flags: needinfo?(mhenretty)
Flags: needinfo?(jfkthame)
Comment on attachment 8560388 [details] [diff] [review] Properly logicalize the implementation of padding on button contents I verified the fix on Nightly (2015-02-11) on OSX. Aurora+
Attachment #8560388 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ah, drat -- that patch was dependent on bug 1079154, which was backed out of Aurora in bug 1121748. OK, here's a modified version of the fix here that avoids that dependency. Flagging for :smontagu to re-review, to make sure I haven't done anything dumb... tested on an Aurora build to ensure that it does make the reftests here pass as expected.
Attachment #8563462 - Flags: review?(smontagu)
Flags: needinfo?(jfkthame)
Comment on attachment 8563462 [details] [diff] [review] [Aurora] Properly logicalize the implementation of padding on button contents Review of attachment 8563462 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsHTMLButtonControlFrame.cpp @@ +362,4 @@ > > // Place the child > + nsPoint physicalPos = > + childPos.GetPhysicalPoint(wm, containerWidth - contentsDesiredSize.Width()); Since this is the only place that we use containerWidth, why not just set it directly to |clbp.LeftRight(wm) + focusPadding.LeftRight(wm)| instead of adding contentsDesiredSize.Width() and then subtracting it? (maybe with an explanatory comment). Or will the compiler optimize that away and it's clearer like this? I'll leave it up to you.
Attachment #8563462 - Flags: review?(smontagu) → review+
(In reply to Simon Montagu :smontagu from comment #20) > Since this is the only place that we use containerWidth, why not just set it > directly to |clbp.LeftRight(wm) + focusPadding.LeftRight(wm)| instead of > adding contentsDesiredSize.Width() and then subtracting it? (maybe with an > explanatory comment). Or will the compiler optimize that away and it's > clearer like this? I'll leave it up to you. Fair point, but in the end I left it as is, mostly on the ground that we have the same containerWidth calculation in the trunk version, and I liked keeping this as similar as possible to trunk. I'd expect the compiler to optimize this sufficiently that there won't be any significant difference. Landed the aurora-rebased version: https://hg.mozilla.org/releases/mozilla-aurora/rev/7a186c7c962d https://hg.mozilla.org/releases/mozilla-aurora/rev/826fe1c67dca
There is a bug for the overlap already written as bug 1132672.
Forgive me this was the wrong bug for comment 23.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: