Closed Bug 1100912 Opened 6 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: wilsonpage, Assigned: smaug)

References

Details

Attachments

(3 files)

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/
*'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)
Blocks: 1217779
I'm not likely to work on this anytime soon. Unassigning.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
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
Attachment #8966772 - Attachment mime type: text/plain → text/html
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.
I guess that is DOM parent node.
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
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)
(Removing dependency from bug 1405937 since directionality state is a DOM thing.)
No longer blocks: shadowdom-layout
Priority: -- → P3
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).
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+
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
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a20a23b5e7
make dir-state to propagate through shadow DOM, r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/18a20a23b5e7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.