Directionality state is propagated in the light tree, not the flattened tree (requires spec)

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: wilsonpage, Assigned: smaug)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

5 years ago
EXPECTED

`:dir()` selector should reflect the closest ancestor's `dir` attribute, even ancestors outside the shadow-host.

ACTUAL

`:dir()` selector only reflects ancestor `dir` attributes inside the shadow-root.

DEMO

http://jsbin.com/talite/1/
Reporter

Comment 1

5 years ago
*'outside the shadow-root'
> even ancestors outside the shadow-host.

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 on the fact that the rendering behavior Chrome and Firefox have here doesn't match what the spec says.

You're right that we're at least not being internally consistent, in that our rendering behavior seems to treat the shadow DOM as having the directionality of the host but the :dir matching does not... but as I said in that spec bug it's not clear to me why the rendering behavior is what it is, and that it's even correct.
Simon, I'm guessing that we don't correctly propagate the directionality content flags down into shadow trees.  Do you know if that should be sufficient here, and whether that is indeed the desired behaviour?
Flags: needinfo?(smontagu)
I'd like to investigate this further but the test case here and in https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 fail for me with "TypeError: div.createShadowRoot is not a function". What am I lacking?

From inspecting the testcases I'm also not clear how they determine whether rendering is LTR or RTL.

My own expectation FWIW is that a shadow root should act like a root and not inherit direction from its container, and certainly that it should be bidi-isolated, but that's just an off the cuff reaction and I haven't looked into it in depth.
Flags: needinfo?(smontagu)
(In reply to Simon Montagu :smontagu from comment #4)
> I'd like to investigate this further but the test case here and in
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 fail for me with
> "TypeError: div.createShadowRoot is not a function". What am I lacking?

Maybe you need to enable web components from about:config ? The pref is "dom.webcomponents.enabled".
From the w3c spec bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=27359 '[we] prefer "dir" to be handled the same way as CSS inherits.' While direction is conceptually distinct from style, I read the discussion there as arriving at concensus that it *should* be inherited down into the shadow root in the same way that CSS properties are. Which I think means that :wilsonpage's assertion in Comment #1 stands and his test case is valid. Is that correct, and if so can we get a plan for fixing this bug?
Flags: needinfo?(bzbarsky)
The spec got clarified, yes.  See comment 3 for what needs to be done to implement that.  All that's needed is someone with time to do it.
Flags: needinfo?(bzbarsky)
TL;DR: this bug makes it very difficult to make BiDi-friendly web components for Gaia.

(In reply to Simon Montagu :smontagu from comment #4)
> My own expectation FWIW is that a shadow root should act like a root and not
> inherit direction from its container, and certainly that it should be
> bidi-isolated

In my opinion, this is not suitable from the front-end development point of view. Many native HTML elements have a different appearance in RTL, e.g. <progress> bars are mirrored horizontally, and we need :dir() selectors to implement some front-end details properly.

Front-end developers can cheat and apply `dir` attributes to elements of the shadow DOM when the web component is created, but this can’t work for elements that are dynamically created. Here’s a typical header bar with a “back” button (that should be mirrored in RTL):

    var div = document.createElement('div');
    div.innerHTML = '<gaia-header action="back"><h1>Gaia Header</h1></gaia-header>';
    document.body.insertBefore(div.firstElementChild, anchor);

in this example, our implementation initializes a <gaia-header> instance in line 2, but the direction of this component could be changed in line 3, depending where the component is inserted in the document. And AFAIK there’s no way to work around this with front-end JS.
In the past, we did workarounds like [1]: listen to attribute mutations on <html> so that we can react. Clearly we're depending on a specific behavior in Gaia here.

[1] https://github.com/gaia-components/gaia-header/pull/11/files#diff-c724cebc352c5f85a50270d4af6309d0R245
Tricks like comment 9 don't seem to work for web components used inside *other* web components. It would be really nice if we could fix this. I don't mind doing it myself if someone points me in the right direction. I have plenty of experience with C++, and know a bit about Gecko dev from working on Thunderbird for several years, but I haven't done anything in Gecko proper.
Boris: do you have any pointers for where I should start looking? I think I could probably handle most of this by myself if I knew where to start.
Flags: needinfo?(bzbarsky)
My guess would be that you want to change RecomputeDirectionality in dom/base/DirectionalityUtils.cpp to use GetParentElementCrossingShadowRoot() instead of GetParentElement().  That might not be the only thing that needs to change, though.

There would be some other changes needed to make dir=auto cross shadow root boundaries in other parts of DirectionalityUtils.cpp and maybe elsewhere, probably in both directions (getting parents and getting children).  I'm not sure whether dir=auto is supposed to work that way or not.

But others might be able to comment more definitively.
Taking this.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
David is right that you need to change RecomputeDirectionality, but it's not clear to me whether it should use GetParentElementCrossingShadowRoot or GetFlattenedTreeParent.  Those have different observable behavior in cases like this:

  <x-foo dir="rtl">
    <span></span>
  </x-foo>

and a shadow tree attached to the x-foo that does:

   <p dir="ltr">
     <children/> <!-- Or whatever the current syntax is -->
   </p>

so that the flattened tree looks like:

   <div dir="rtl">
     <p dir="ltr">
       <span></span>
     </p>
   </div>

Using GetParentElementCrossingShadowRoot() will give the <span> a directionality of "rtl", but using GetFlattenedTreeParent() will give it "ltr".  I suspect you actually want GetFlattenedTreeParent().

You will also need to change SetDirectionalityOnDescendants to walk the flattened tree using FlattenedChildIterator, presumably.  Not sure what should happen for nodes that appear in the DOM but not the flattened tree; I guess you get to check the spec for that and hope whoever wrote it actually defined the behavior.

Making "auto" work will need changes to WalkAncestorsResetAutoDirection (again, to replace GetParentElement()) and The various WalkDescendants* methods in DirectionalityUtils.

And you'll need to make sure that (1) SetDirOnBind happens correctly for all the things we care about and (2) changes to insertion points that change the flattened tree somehow recompute directionalities as needed.  I haven't thought through what's involved in this so far.

Make sure to write lots of tests, especially for dynamic update cases....
Flags: needinfo?(bzbarsky)

Updated

4 years ago
Blocks: 1217779
I'm not likely to work on this anytime soon. Unassigning.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW

Updated

a year ago
This is actually more of a DOM thingie.
Actually in chrome the test-case in comment 17 doesn't work at all because they don't support dir, presumably.
Summary: [css] :dir() doesn't work as expected in shadow-dom → Directionality state is propagated in the light tree, not the flattened tree
Assignee

Updated

a year ago
Attachment #8966772 - Attachment mime type: text/plain → text/html
Assignee

Comment 19

a year ago
HTML spec says
"Otherwise, the directionality of the element is the same as the element's parent element's directionality."
But element's parent isn't actually defined. Which parent.
Assignee

Comment 20

a year ago
I guess that is DOM parent node.
Assignee

Comment 21

a year ago
As far as I see, this is totally unclear from the spec. (or I've missed to look at some other place defining this)
Filed https://github.com/whatwg/html/issues/3699
Assignee

Updated

a year ago
Summary: Directionality state is propagated in the light tree, not the flattened tree → Directionality state is propagated in the light tree, not the flattened tree (requires spec)
Assignee

Comment 22

a year ago
(Removing dependency from bug 1405937 since directionality state is a DOM thing.)
No longer blocks: shadowdom-layout
Depends on: 1469108

Updated

11 months ago
Priority: -- → P3
Assignee

Comment 23

10 months ago
Since no one has managed to come up with an algorithm which would implement the behavior people actually want, I'll upload a patch which mimics blink's behavior in most cases (follows flattened-ish tree model).
Assignee

Comment 24

10 months ago
This is a nightmare patch to make the nightmare code DirectionalityUtils to deal with ShadowDOM in a way which isn't specified anywhere :)

The patch tries to mimic flattened tree in DOM. So dir state propagates through shadow DOM and from slots back to light dom child nodes.
dir=auto detection on the other hand propagates from assigned nodes and shadow DOM up to the host.

and :dir(rtl)/:dir(ltr) states are propagated too. (Other engines don't support :dir pseudoclass yet.)

I should write more tests and I'm sure there are plenty bugs, like directionality handling always has, but trying to get something landed sooner than later
(and I should be on vacation), and then improve it later.

(DirectionalityUtils in general could use a total rewrite, but not worth right now since it is expected that Shadow DOM's dir handling will change - well - be specified at least in some way.)

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/5907c5d7157f05a49da06c0cef662d1a21bbf5b5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5907c5d7157f05a49da06c0cef662d1a21bbf5b5
remote: recorded changegroup in replication log in 0.052s
Assignee: nobody → bugs
Attachment #8992228 - Flags: review?(mrbkap)
Comment on attachment 8992228 [details] [diff] [review]
shadow_directionality_with_test.diff

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

I think I understand enough to r+ this.
Attachment #8992228 - Flags: review?(mrbkap) → review+
Assignee

Comment 26

10 months ago
ops, one line change in the existing code caused some crashes
That was done while debugging new code.
SetAncestorDirectionIfAuto shouldn't use SubtreeRoot but keep using parent.

Let's see if this is more tryserver happy. Just that one change.

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/5eda3cc6054270e833017b3addba725c869aec16
remote:   https://hg.mozilla.org/try/rev/763b5cdf02e7da07a3b30b29611f38d660427dbc
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=763b5cdf02e7da07a3b30b29611f38d660427dbc
remote: recorded changegroup in replication log in 0.007s

Comment 27

10 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a20a23b5e7
make dir-state to propagate through shadow DOM, r=mrbkap

Comment 28

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18a20a23b5e7
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.