1119475 - convert UA stylesheets to use logical rather than physical properties

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 (WIP) - Replace physical with logical properties in html.css

Here's an (untested) beginning... changing properties in html.css. We'll need to do similar things with the other CSS files, too. And test them extensively.

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8546152 [details] [diff] [review]
pt 1 (WIP) - Replace physical with logical properties in html.css

> /* titles */
> abbr[title], acronym[title] {
>-  border-bottom: dotted 1px;
>+  border-block-end: dotted 1px;
> }

I wonder if we really should be using a kind of text decoration for this, since border-block-end is not going to be right for sideways text, at least.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Indeed; I suspect it was done the way it was because it predates text-decoration-style.

Comment 4

[Cameron McCormack (:heycam)]

Checking the HTML spec, I see:

  abbr[title], acronym[title] { text-decoration: dotted underline; }

in the Rendering chapter.

I am a little worried that switching to text-decoration would cause problems with pages that write border:none to turn off the dotted line.  Though maybe authors are already doing border:none;text-decoration:none; to cover other browsers too.

Comment 5

[Cameron McCormack (:heycam)]

Having just checked with http://mcc.id.au/temp/abbr.html, I find to my surprise that none of Chrome, Safari and IE render a dotted line under abbrs and acronyms.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8546152 [details] [diff] [review]
pt 1 (WIP) - Replace physical with logical properties in html.css

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

::: layout/style/html.css
@@ +419,5 @@
> +table[align="center"] > caption[align="left"]:-moz-dir(ltr) {
> +  margin-inline-end: 0;
> +}
> +table[align="center"] > caption[align="left"]:-moz-dir(rtl) {
> +  margin-inline-start: 0;

The logicalized rules for table caption alignment are interesting.  This makes <caption align=left> mean left in horizontal ltr, left in horizontal rtl, but top in vertical ltr.  How are we doing interpreting the align attribute when doing the actual table layout and placing the caption frame in non-horizontal-ltr writing modes?

Comment 7

[Cameron McCormack (:heycam)]

Attachment: tests WIP (v0.1)

Here are tests that cover each rule in the UA style sheet that attachment 8546152 [details] [diff] [review] changes.  From testing with my logical properties patches applied, the only real problem I find is that the abbr dotted border rules aren't applied.  Maybe that's a problem with my logical border property patches.

(This is ignoring other issues with vertical text, such as table layout not really working and marquees fatally asserting.)

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> From testing with my logical
> properties patches applied, the only real problem I find is that the abbr
> dotted border rules aren't applied.  Maybe that's a problem with my logical
> border property patches.

Oh, I neglected to implement the border-{block,inline}-{start,end} shorthands.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #7)
> (This is ignoring other issues with vertical text, such as table layout not
> really working 

Yes, tables are simply not implemented yet (they're bug 1077521). We might even want to put

  writing-mode:horizontal-tb !important;

on the table elements for the time being.

> and marquees fatally asserting.)

Ugh, we definitely need to fix that.

(In reply to Cameron McCormack (:heycam) from comment #6)

> The logicalized rules for table caption alignment are interesting.  This
> makes <caption align=left> mean left in horizontal ltr, left in horizontal
> rtl, but top in vertical ltr.  How are we doing interpreting the align
> attribute when doing the actual table layout and placing the caption frame
> in non-horizontal-ltr writing modes?

We'll consider this in bug 1077521 (or a dependency of it) once we get around to tackling tables.

Comment 10

[Cameron McCormack (:heycam)]

I pushed a try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=8182ff699996 which shows a few reftest failures.

Comment 11

[Cameron McCormack (:heycam)]

Those reftest failures are because the UA style sheet is using the margin-inline-start etc. properties, which are aliases of -moz-margin-start etc., and there's no way currently to give aliases flags like CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS.  So I think for now we should use the -moz prefixed versions of the inline logical properties.

Comment 12

[Cameron McCormack (:heycam)]

An updated try run with bug 1121768 applied and with the comment 11 changes is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=281211417d11

Running the reftests I wrote locally I think your html.css WIP patch just needs the comment 11 changes and it's ready.  I think it would be fine to leave the discussion of what to do with abbr's dotted underline to a separate bug.

I will look into the other UA style sheets using the existing logical properties, which looks to be just forms.css and quirk.css.

Comment 13

[Cameron McCormack (:heycam)]

Actually, it's more than just those two files, since I should've been looking for existing uses of physical properties.  And I think Jonathan you probably have a better idea of what's going to happen with form controls so perhaps I will leave the style sheet changes to you. :-)

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: testcase showing <caption align=...> behavior

(In reply to Cameron McCormack (:heycam) from comment #6)
> The logicalized rules for table caption alignment are interesting.  This
> makes <caption align=left> mean left in horizontal ltr, left in horizontal
> rtl, but top in vertical ltr.

The behavior of <caption align=...> is a bit strange, actually. Your reference for ua-style-sheet-margin-9.html does not match our *existing* behavior, which is that when the caption has display:block, setting align=left will make it *right*-aligned (regardless of dir) and vice versa. (See attachment.)

I'm guessing that the use of display:block with <caption> is unexpected, and the attribute was not implemented with this in mind.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Replace physical with logical properties in html.css

Updated as per comment 12. Let's leave the <abbr> underline to be considered separately; we're also going to want some enhancements for other underline-related issues in vertical text anyway, I believe. The aim of this bug is just to convert our existing styles to use logical properties but otherwise maintain existing behavior; where we need to adapt/enhance behavior of certain features, we'll file specific new bugs.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Replace physical with logical properties in forms.css

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Replace physical with logical properties in quirk.css

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Replace physical with logical properties in contenteditable.css

Comment 19

[Jonathan Kew [:jfkthame]]

The above patches appear to be green on try with vertical writing modes *not* enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e76523d2edb1.

With vertical support enabled, some of your new tests don't currently pass, so I'm intending to look into those issues further. Some may relate to features that aren't yet implemented (e.g. tables), and some may need modifications/corrections to the tests.

Comment 20

[Cameron McCormack (:heycam)]

Will you write tests for the rules you've converted in these other style sheets, or would you like me to take a look at that?

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can we please get the spec updated and tests for all those submitted to web-platform-tests?

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #20)
> Will you write tests for the rules you've converted in these other style
> sheets, or would you like me to take a look at that?

If you have (or can find) the time, that would be awesome -- thanks. I'll be starting to go through your existing WIP tests to see what issues they throw up, but obviously we also need to extend that coverage to the additional converted rules.

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8551489 [details] [diff] [review]
pt 2 - Replace physical with logical properties in forms.css

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

::: layout/style/forms.css
@@ -17,3 @@
>    overflow: inherit;
>    padding: inherit;
>    height: 100%;   /* Need this so percentage heights of kids work right */

I don't know what this is for exactly, but it sounds like it might need to be block-size for vertical fieldsets?  Have you tested this?

@@ +362,5 @@
>     * We can't change the padding here, because that would affect our
>     * intrinsic width, since we scroll.  But at the same time, we want
>     * to make sure that our left border+padding matches the left
>     * border+padding of a combobox so that our scrollbar will line up
>     * with the dropmarker.  So set our left border to 2px.

Update this comment not to talk about "left" borders.

@@ +469,5 @@
>  input[type="color"]:-moz-system-metric(color-picker-available)::-moz-color-swatch {
> +  inline-size: 100%;
> +  block-size: 100%;
> +  min-inline-size: 3px;
> +  min-block-size: 3px;

Do you think it's better to use the logical properties here?  Obviously it's not necessary.

@@ +481,5 @@
>  /* Try to make RTL <input type='file'> look nicer. */
>  /* TODO: use text-align: match-parent when bug 645642 is fixed. */
>  input[type="file"]:-moz-dir(rtl) > xul|label {
>    -moz-padding-start: 0px;
> +  -moz-padding-end: 5px;

Do you what the old rules here, with mixed -moz-padding-start and padding-right, were trying to achieve?

@@ +499,5 @@
> +}
> +input[type="radio"]:-moz-dir(rtl) {
> +  -moz-margin-start: 3px;
> +  -moz-margin-end: 5px;
> +}

So this means "ignore ltr/rtl, but still take account of whether we're in a vertical writing mode", is that right?  Does that look better than just using

  -moz-margin-start: 5px;
  -moz-margin-end: 3px;

for all writing modes?

@@ +578,5 @@
>  input[type="reset"],
>  input[type="button"],
>  input[type="submit"] {
>    -moz-appearance: button;
>    /* The sum of border-top, border-bottom, padding-top, padding-bottom

Logicalise the comment?

@@ +610,5 @@
>  }
>  
>  input[type="color"]:-moz-system-metric(color-picker-available) {
> +  inline-size: 64px;
> +  block-size: 23px;

BTW I assume you'll be doing the work to make the color picker button (and other form control things with specific sizes) rotate its content in another bug?

@@ +648,5 @@
>  input[type="submit"]:active:hover {
> +  padding-block-start: 0px;
> +  -moz-padding-end: 5px;
> +  padding-block-end: 0px;
> +  -moz-padding-start: 7px;

Are you sure these should be logicalised?  The corresponding padding rules for the non-:hover/non-:active aren't.

@@ +669,5 @@
>  input[type="file"] > button[type="button"]::-moz-focus-inner {
> +  padding-block-start: 0px;
> +  -moz-padding-end: 2px;
> +  padding-block-end: 0px;
> +  -moz-padding-start: 2px;

Same here.

@@ +698,5 @@
>       must be the same here and for text inputs */
> +  padding-block-start: 0px;
> +  -moz-padding-end: 6px;
> +  padding-block-end: 0px;
> +  -moz-padding-start: 6px;

And here.

@@ +834,5 @@
>  
>  input[type=range] {
>    -moz-appearance: range;
>    display: inline-block;
> +  width: 12em; /* XXX how does orient attribute relate to vertical mode? */

It's a good question.  I don't actually see the orient attribute in the HTML spec; it seems to assume the orient is assumed based on whether the width or height of the element is bigger.

@@ +970,5 @@
>  input[type=number]::-moz-number-spin-box {
>    display: flex;
>    flex-direction: column;
>  %ifdef XP_WIN
>    /* The Window's Theme's spin buttons have a very narrow minimum width, so

Update comment?

@@ +992,5 @@
>    background-position: center bottom;
>    border: 1px solid darkgray;
>    border-bottom: none;
> +  /* [JK] I think the border-*-*-radius properties here can remain physical,
> +     as we probably don't want to turn the spinner sideways in vertical writing mode */

My gut feeling is that they should be positioned like this:

  +-----+
  |     |
  |     |
  |     |
  |     |
  |(v|^)|
  +-----+

or perhaps with the two buttons swapped.  But this is just a feeling, I'm not sure it's principled.

::: layout/style/number-control.css
@@ +12,5 @@
>    /* Has to revert some properties applied by the generic input rule. */
>    -moz-binding: none;
> +  inline-size: 149px; /* to match type=text */
> +  /* XXX Should this *really* be an absolute number of pixels?
> +     Would a dimension specified in ch or em be better? */

Where is the generic <input type=text> width coming from anyway?  I can't see it in forms.css.

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8551490 [details] [diff] [review]
pt 3 - Replace physical with logical properties in quirk.css

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

::: layout/style/quirk.css
@@ +192,4 @@
>  }
>  
> +img[align=right]:-moz-dir(ltr), img[align=left]:-moz-dir(rtl) {
> +  -moz-margin-start: 3px;

It's unclear to me what quirky <img align=left> should mean if you put it in a vertical writing mode context.  It looks here like you're interpreting it as top?  I guess this is OK, but I'm not sure whether it's a good idea to make quirk.css more useful. :-)

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8551491 [details] [diff] [review]
pt 4 - Replace physical with logical properties in contenteditable.css

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

::: layout/style/contenteditable.css
@@ +199,1 @@
>    background-image: url("resource://gre/res/grabber.gif");

This one is square, so it's OK...

@@ +210,5 @@
>    z-index: 2147483647; /* max value for this property */
>    text-decoration: none !important;
>    border: none 0px !important;
> +  inline-size: 4px;
> +  block-size: 8px;

...but this one isn't square.  Will you be somehow rotating the background image?  Is there even a way to do that from CSS based on the writing mode?

(Same comment applies to the rest of the changes in this file.)

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8551489 [details] [diff] [review]
pt 2 - Replace physical with logical properties in forms.css

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

::: layout/style/forms.css
@@ +481,5 @@
>  /* Try to make RTL <input type='file'> look nicer. */
>  /* TODO: use text-align: match-parent when bug 645642 is fixed. */
>  input[type="file"]:-moz-dir(rtl) > xul|label {
>    -moz-padding-start: 0px;
> +  -moz-padding-end: 5px;

Note that the label in this case still has direction:ltr, thanks to the "input[type="file"] > xul|label" block above (see comment "Force the text to have LTR directionality.")

Therefore, the rules achieved the same thing: cancel the padding on the left, and put it on the right instead. It was just written in a confusing way.

@@ +499,5 @@
> +}
> +input[type="radio"]:-moz-dir(rtl) {
> +  -moz-margin-start: 3px;
> +  -moz-margin-end: 5px;
> +}

This was just intended to maintain the existing rendering for horizontal modes, both ltr and rtl.

Personally, I think it may actually look better to use the simpler version you suggest; but I'll run it through try and see if it upsets any existing tests.

@@ +515,5 @@
> +}
> +input[type="checkbox"]:-moz-dir(rtl) {
> +  -moz-margin-start: 3px;
> +  -moz-margin-end: 4px;
> +}

A similar consideration may apply here. In this case, there is an existing reftest that fails if we use the simpler approach, but I don't think the test was really meant to care about the left/right margins, so perhaps we can fix that.

@@ +610,5 @@
>  }
>  
>  input[type="color"]:-moz-system-metric(color-picker-available) {
> +  inline-size: 64px;
> +  block-size: 23px;

Its content is a simple color swatch, so it doesn't seem to need any further work to "rotate" it -- except when native theming applies, but that's a whole other can of worms that's still to be addressed...

(Yes, there are other form controls where content also needs work. Some such bugs are already on file, and I expect there'll be more.)

@@ +648,5 @@
>  input[type="submit"]:active:hover {
> +  padding-block-start: 0px;
> +  -moz-padding-end: 5px;
> +  padding-block-end: 0px;
> +  -moz-padding-start: 7px;

Where are you seeing the corresponding rules that aren't logicalized? I must be missing something.....

@@ +834,5 @@
>  
>  input[type=range] {
>    -moz-appearance: range;
>    display: inline-block;
> +  width: 12em; /* XXX how does orient attribute relate to vertical mode? */

We have an existing bug about this, fwiw, but no clear decision as to exactly how to move forward.

@@ +970,5 @@
>  input[type=number]::-moz-number-spin-box {
>    display: flex;
>    flex-direction: column;
>  %ifdef XP_WIN
>    /* The Window's Theme's spin buttons have a very narrow minimum width, so

Actually, I'm inclined to revert the changes here for now, and consider it when nsNumberControlFrame gets adapted.

@@ +992,5 @@
>    background-position: center bottom;
>    border: 1px solid darkgray;
>    border-bottom: none;
> +  /* [JK] I think the border-*-*-radius properties here can remain physical,
> +     as we probably don't want to turn the spinner sideways in vertical writing mode */

I was assuming they'd remain one above the other, retaining the clear "up/down" positional semantics, but your suggestion is also a possibility. If we did do that, I guess we would need logical border-radius properties here.

In any case, we can revisit this when nsNumberControlFrame gets some vertical-mode love. I filed bug 1123299 for that.

::: layout/style/number-control.css
@@ +12,5 @@
>    /* Has to revert some properties applied by the generic input rule. */
>    -moz-binding: none;
> +  inline-size: 149px; /* to match type=text */
> +  /* XXX Should this *really* be an absolute number of pixels?
> +     Would a dimension specified in ch or em be better? */

The default width of an <input type=text> will be based on its cols attribute (if any, otherwise there's a default of 20, IIRC) and the character width the font it's using. I guess the magic "149px" here happened to give the intended result on one particular system at the time someone wrote it, but it's virtually guaranteed NOT to be right in general.

So I think this is fundamentally bogus. But that should be fixed in its own bug, with input from whoever cares about it... the idea here is just to maintain existing behavior, while allowing vertical mode to do something reasonable.

In fact, on digging into history a little, it looks like this originated in bug 635240. So I'll needinfo jwatt to see if he can justify the number. :)

Comment 27

[Jonathan Kew [:jfkthame]]

jwatt, please see the question immediately above re the width of <input type=number>. I don't believe this works quite as intended in any case, and the result (in comparsion with type=text) varies substantially across platforms anyway. Would you see any objection to replacing the "149px" with something like "20ch" or thereabouts?

Comment 28

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Would you see any objection to replacing the "149px" with
> something like "20ch" or thereabouts?

I think that would be better.

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8551491 [details] [diff] [review]
pt 4 - Replace physical with logical properties in contenteditable.css

Actually, to do the contenteditable stuff right, we're going to need to modify how the table-editing code references these resources. Cancelling this patch, until we actually tackle vertical-mode tables.

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Replace physical with logical properties in forms.css

Updated following review comment 23, and responses in comment 26 etc.

Comment 31

[Cameron McCormack (:heycam)]

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> @@ +648,5 @@
> >  input[type="submit"]:active:hover {
> > +  padding-block-start: 0px;
> > +  -moz-padding-end: 5px;
> > +  padding-block-end: 0px;
> > +  -moz-padding-start: 7px;
> 
> Where are you seeing the corresponding rules that aren't logicalized? I must
> be missing something.....

Not sure what I was looking at now; please ignore.

Comment 32

[Jonathan Kew [:jfkthame]]

Attachment: pt 0 - Ensure logical properties are enabled in UA sheets

Before we can consider landing any of these stylesheet changes, we need to ensure the logical properties are enabled for UA sheets. Do you have a pending patch in some other bug for this already? If not, I suggest we do it as 'part 0' here.

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8559423 [details] [diff] [review]
pt 0 - Ensure logical properties are enabled in UA sheets

No I didn't upload a patch for this.

Comment 34

[Jonathan Kew [:jfkthame]]

Attachment: tests pt 1 - Reftests for logical properties used in UA stylesheets

This is an updated copy of your tests from attachment 8548825 [details] [diff] [review]. In particular, I've made them use the dir attribute for RTL, rather than the CSS direction property, as that better reflects the main use case (and note that it behaves differently with regard to :-moz-dir() in selectors); and I've disabled a few parts that will currently fail in vertical mode because we don't support the relevant constructs yet.

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: tests pt 2 - More reftests for logical properties in UA stylesheets (form controls)

And here are additional tests for some of the form-element rules. Note that the more complex elements like <select> are not yet supported in vertical mode at all (for now, it'll have 'writing-mode:horizontal-tb' in forms.css), so I haven't yet made tests for that.

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8560880 [details] [diff] [review]
tests pt 1 - Reftests for logical properties used in UA stylesheets

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

Can you either file a bug for the disabled table bits of the tests and mention it in the comment, or maybe refer to an existing bug for logicalising tables and also comment in that bug about fixing the tests?

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Replace physical with logical properties in content.css for android and b2g

It turns out that android and b2g have their own versions of padding for some elements, so we need to adapt those styles as well. And this means they need a distinct reference for a couple of the tests.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: tests pt 3 - Modify reftest for textarea padding on Windows Vista and later

And one more platform-specific adjustment: on Vista and later, the code in nsNativeThemeWin::GetWidgetPadding sets 2px padding on all sides of textarea, so we need a different reference for that testcase. With this fix, it looks like tryserver might finally give me a green reftest run with vertical writing-mode enabled and with all the patches in this bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb01138dfc7.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Replace physical with logical properties in content.css for android and b2g

Tryserver pointed out that I missed a rule in b2g's contents.css. Updated pt 4 to fix that. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3afe637cd226

Comment 40

[Cameron McCormack (:heycam)]

Two things I noticed which I assume you will fix in separate bugs:

(a) the inline-size of the textarea in vertical-rl mode looks too big
(b) the padding inside the natively themed buttons in vertical-rl mode looks wrong

Comment 41

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #40)
> (b) the padding inside the natively themed buttons in vertical-rl mode looks
> wrong

I guess that might not be padding but just reflow which needs to be fixed at one point.  The actual UA style rule conversion look OK.

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8561674 [details] [diff] [review]
pt 4 - Replace physical with logical properties in content.css for android and b2g

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

Do we need to logicalise the |select[disabled] > button| rule?

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #41)
> (In reply to Cameron McCormack (:heycam) from comment #40)
> > (b) the padding inside the natively themed buttons in vertical-rl mode looks
> > wrong
> 
> I guess that might not be padding but just reflow which needs to be fixed at
> one point.  The actual UA style rule conversion look OK.

I think the issue there is that we're still getting horizontal styling from the native theme code. Either we need to get appropriate native styling for vertical widgets (if such exists), or perhaps we need to disable native theming (when writing-mode is vertical) for widgets where the platform doesn't provide such support. Either way, that's a separate bug in layout and/or widget code.

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #42)
> Comment on attachment 8561674 [details] [diff] [review]
> pt 4 - Replace physical with logical properties in content.css for android
> and b2g
> 
> Review of attachment 8561674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we need to logicalise the |select[disabled] > button| rule?

Eventually, yes. But currently <select> elements don't support vertical mode at all (that's bug 1112954), and to avoid complete breakage, we're applying "writing-mode:horizontal-tb !important" to them. So I've left this for now.

Comment 45

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8bf120bfa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/189f01bd5361
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b55a575eef
https://hg.mozilla.org/integration/mozilla-inbound/rev/130e64386759
https://hg.mozilla.org/integration/mozilla-inbound/rev/67bdeca69751
https://hg.mozilla.org/integration/mozilla-inbound/rev/193adf744f28
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb88a2c2b950
https://hg.mozilla.org/integration/mozilla-inbound/rev/44476ddac532

Comment 46

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #44)
> (In reply to Cameron McCormack (:heycam) from comment #42)
> > Do we need to logicalise the |select[disabled] > button| rule?
> 
> Eventually, yes. But currently <select> elements don't support vertical mode
> at all (that's bug 1112954), and to avoid complete breakage, we're applying
> "writing-mode:horizontal-tb !important" to them. So I've left this for now.

Can we fix that sooner rather than later? I've been working on vertical <select>s in bug 1113206, and I've been waiting on this bug to sort out the parts of the broken behaviour that come from the stylesheet before carrying on with code fixes. If the stylesheet fixes are also waiting for the code fixes, we won't make much progress ;-)

Comment 47

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #46)
> Can we fix that sooner rather than later? I've been working on vertical
> <select>s in bug 1113206, and I've been waiting on this bug to sort out the
> parts of the broken behaviour that come from the stylesheet before carrying
> on with code fixes. If the stylesheet fixes are also waiting for the code
> fixes, we won't make much progress ;-)

Ah, OK. I'd suggest we do any further stylesheet fixes needed in bug 1113206 alongside the code changes. For a start, you'll want to remove the recently-added "writing-mode:horizontal-tb !important" from the |select| style in forms.css. It looks like the main <select> properties are logicalized at this point, so you should be able to move forward from there.

I figured the time to examine such things as <select> styling properly would be at the same time as writing tests. Though on re-reading the files, it looks like virtually all of them have in fact been updated, at least to a first approximation -- there may be less residue than I thought.

Comment 48

[Jonathan Kew [:jfkthame]]

Oops... a couple of the test files were accidentally omitted from the patches when pushing to inbound. (This didn't cause failures only because the writing-modes test directory is not yet included in standard reftest runs.)

Pushed the missing files as a followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecb0ebbbe89

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f8bf120bfa0
https://hg.mozilla.org/mozilla-central/rev/189f01bd5361
https://hg.mozilla.org/mozilla-central/rev/84b55a575eef
https://hg.mozilla.org/mozilla-central/rev/130e64386759
https://hg.mozilla.org/mozilla-central/rev/67bdeca69751
https://hg.mozilla.org/mozilla-central/rev/193adf744f28
https://hg.mozilla.org/mozilla-central/rev/cb88a2c2b950
https://hg.mozilla.org/mozilla-central/rev/44476ddac532
https://hg.mozilla.org/mozilla-central/rev/3ecb0ebbbe89