261361 - View -> Use style -> None (Firefox: Page Style -> None) should not clear direction

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Asaf Romano (gone)]

When I'm clearing page style (becuase of bad color scheme, fonts, etc.) I don't
expect BiDi text to become unreadable.

Step to repro':
1. Visit RTLed page
2. Clear Page Style (View -> Page Style -> None)

Results: document direction is lost, mixed-direction text is not readable.

Comment 1

[Asaf Romano (gone)]

http://lxr.mozilla.org/aviarybranch/source/content/base/src/nsStyleSet.cpp#127

Comment 2

[Eyal Rozenberg]

Are there currently any other style attributes which are preserved when styles
are cleared?

Also, alternative solution to not clearing the direction style rules at all
would be to only maintain the page's direction, or perhaps even to apply
direction auto-detection to each block element/paragraph/etc.

Comment 3

[fantasai]

No, the entire cascading level gets turned off.
What we can do, though, is to create a pair of back-up rules in html.css.
  [dir="left"] { direction: ltr; unicode-bidi: embed; }
  [dir="right"] { direction: rtl; unicode-bidi: embed; }
Those will continue to take effect.

Comment 4

[Hixie (not reading bugmail)]

dir="" is not a stylistic attribute. (Neither is lang="", which is also mapped
to style in Mozilla -- does the No Style patch break our Properties window's
description of the language?)

Comment 5

[Eyal Rozenberg]

I don't see how dir="rtl" is any more or less stylistic than style="direction:
rtl"...

Comment 6

[Hixie (not reading bugmail)]

See http://www.w3.org/Style/Group/css2-src/cascade.html#q13

Comment 7

[Anne (:annevk)]

Ian means: <http://www.w3.org/TR/2004/CR-CSS21-20040225/cascade.html#q13> I guess.

Comment 8

[Asaf Romano (gone)]

(In reply to comment #4)
> dir="" is not a stylistic attribute. (Neither is lang="", which is also mapped
> to style in Mozilla -- does the No Style patch break our Properties window's
> description of the language?)

It looks as the lang att. isn't broken.

Comment 9

[Asaf Romano (gone)]

(In reply to comment #3)
> No, the entire cascading level gets turned off.
> What we can do, though, is to create a pair of back-up rules in html.css.
>   [dir="left"] { direction: ltr; unicode-bidi: embed; }
>   [dir="right"] { direction: rtl; unicode-bidi: embed; }
> Those will continue to take effect.
> 

I suppose you meant: 
[dir="rtl"] {direction: rtl;}
[dir="ltr"] {direction: ltr;}

Comment 10

[Asaf Romano (gone)]

Attachment: Is it the right fix?

per comment 3 and comment 9.

While this does fix it, i'm not sure if the "right" one.

fantasai?

Comment 11

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

We need to make only some of nsHTMLStyleSheet be disabled.

Comment 12

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

(And also probably some of html.css should be disabled.)

Comment 13

[fantasai]

David,
We shouldn't need to mess around with exceptions. CSS2.1's cascade should be
aligned so that all the presentational effects we need to disable (and only the
effects we need to disable) are cascaded into the author level. If it's not,
then that's a problem in CSS2.1 which we should address.

Asaf,
You need to set unicode-bidi to 'embed' because 'dir' introduces an embedding.
http://www.w3.org/TR/html40/struct/dirlang.html#h-8.2.3

Comment 14

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

OK, so based on what CSS2.1 currently says, we need to split nsHTMLStyleSheet
somehow so some of it is at the UA level and some is at the pres hint level.

Comment 15

[fantasai]

We've got bug 252530 open on having a CSS preshint sheet for moving rules into
from html.css.

Which rules do we want to move from nsHTMLStyleSheet into the UA level?

Comment 16

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

Everything that http://www.w3.org/TR/2004/CR-CSS21-20040225/cascade.html#q13
says we should move.  But it probably requires changing a good bit of how
nsHTMLStyleSheet works.

Comment 17

[fantasai]

Attachment: proposed patch

Comment 18

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

Comment on attachment 164278 [details] [diff] [review]
proposed patch

I guess I'm willing to accept this approach, but you also need to remove
BdoMapAttributesIntoRule from nsHTMLSpanElement.cpp.

Comment 19

[fantasai]

Attachment: patch

Comment 20

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

Comment on attachment 167172 [details] [diff] [review]
patch

r=dbaron if:
 * you remove nsHTMLSpanElement::GetAttributeMappingFunction entirely
 * you back out if there's a Tp regression on btek

Comment 21

[fantasai]

Attachment: patch

Comment 22

[fantasai]

I can't check in or back out, so someone else will have to cover that.
Who should I ask for sr?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167214 [details] [diff] [review]
patch

sr=bzbarsky if you also remove the declaration of GetAttributeMappingFunction
in the nsHTMLSpanElement class.  I can check this in for you, once you post the
updated patch.	Thanks for doing this, fantasai!

Comment 24

[fantasai]

Attachment: patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to layout reorg

Comment 26

[Boris Zbarsky [:bzbarsky]]

Patch checked in to trunk.