Closed Bug 1451576 Opened 7 years ago Closed 6 years ago

[dir=auto] doesn't detect rtl text inside Native Anonymous Content

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed
firefox62 --- wontfix

People

(Reporter: bgrins, Assigned: timdream)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1451576 - Demo of using [dir] in NAC
59 bytes, text/x-review-board-request
Details
Bug 1451576 - Allow dir=auto to work in native anonymous content
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1451576 - Add dir=auto to subtitle menu items in video control
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1451576 - Set dir=auto on [Browse...] button of <input type=file>
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
This is something I noticed in Bug 1446830. I'll push up a patch to make it easier to reproduce.
Ehsan, this is a more minimal testcase of what I'm seeing from Bug 1446830. Does this reproduce for you? STR: * Apply the patch * ./mach run --devtools --setpref devtools.inspector.showAllAnonymousContent=true --setpref devtools.inspector.showUserAgentStyles=true 'data:text/html,<input type="file" />' * Pick a file with RTL text When I inspect the label in devtools I see: ``` -moz-dir-attr-like-auto:dir(ltr) { direction: ltr; } ``` Something interesting is that if I open `data:text/html,<input dir="rtl" type="file" />` and then inspect the "Browse" button, it does properly detect LTR directionality. Wondering if this has to do with the text being set initially vs being updated dynamically.
Flags: needinfo?(ehsan)
(In reply to Brian Grinstead [:bgrins] from comment #2) > Something interesting is that if I open `data:text/html,<input dir="rtl" > type="file" />` and then inspect the "Browse" button, it does properly > detect LTR directionality. Wondering if this has to do with the text being > set initially vs being updated dynamically. That the bug happens only when the text is set dynamically makes perfect sense, and is a *strong* indication that there is a bug somewhere in DirectionalityUtils.cpp code which confuses us when dealing with native anonymous content. Andrew, any chance you could help find an owner for this to help unblock Brian please? Thanks.
Flags: needinfo?(ehsan) → needinfo?(overholt)
The only mention within DirectionalityUtils.cpp on NAC is the change made in bug 839886, in DoesNotParticipateInAutoDirection(). My guess is that aElement->IsInAnonymousSubtree() check should move to DoesNotAffectDirectionOfAncestors() and/or NodeAffectsDirAutoAncestor(), since it looks like what we want here is to have <button dir=auto> have it's own direction.
... and bug 1377826 but it might be less related.
This is a more involved patch that does what's needed for comment 1 to work. But it basically reverted bug 1377826, so we will need to dig into layout to find an alternative fix to bug 1377826.
Seems Tim is heroically taking a look here.
Flags: needinfo?(overholt)
Attachment #8968234 - Attachment is obsolete: true
The attached patch now works with Brian's demo here, and my second cset that adds dir=auto to the button on <input type=file>. It also limits itself enough to not crash the test case in bug 1377826. I still don't think this is the exhaust fix, because when I try to add dir=auto to the other place I think we need -- the subtitle track list items [1] -- I was met with "These should always be in sync!" assertion error [2]. [1] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/content/widgets/videocontrols.xml#1658 [2] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/dom/base/nsINode.cpp#313 On the other hand, if this is good enough to unblock bug 1446830, it should be good enough -- I don't think we need an auto-direction implementation that works on every bit of anonymous content, given that we control the markup (or, do we?). Brian, could you tell me where you intend to set dir=auto in bug 1446830? I didn't find it in the cset you posted.
Comment on attachment 8968467 [details] Bug 1451576 - Allow dir=auto to work in native anonymous content Ehsan, could you tell me if I am going in the right direction here? Let me know if you couldn't and someone else could. Thanks.
Attachment #8968467 - Flags: feedback?(ehsan)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12) > On the other hand, if this is good enough to unblock bug 1446830, it should > be good enough -- I don't think we need an auto-direction implementation > that works on every bit of anonymous content, given that we control the > markup (or, do we?). > > Brian, could you tell me where you intend to set dir=auto in bug 1446830? I > didn't find it in the cset you posted. Pushed up the latest version of the patch there. The idea with that version is the markup in the NAC would look something like: ``` <div> <button dir="auto" /> <div> <label dir="auto"> <span>${aValue}</span> <-- First half (when overflowing) <span><span>${aValue}</span></span> <-- Second half (when overflowing) ${aValue} <-- Full string (when not overflowing) </label> </div> </div> ``` But we could change it as needed to work with whatever fix makes sense here.
Attachment #8968468 - Attachment is obsolete: true
Comment on attachment 8968467 [details] Bug 1451576 - Allow dir=auto to work in native anonymous content The markup in comment 14 will also trigger assertion failure described in comment 12. I look it up a bit but I don't understand the code enough to tell why it happens. I couldn't find anything obvious where HasDirAuto could affect the way we call CharacterData::UnbindFromTree(). When it happens, |slowNode| points to the node with dir=auto and |node| points to the outer native anon root <div>. This explains why if the element set with dir=auto is happen to be the direct children of <input> (i.e. they are native anonymous roots), we won't hit this assertion.
Attachment #8968467 - Flags: feedback?(ehsan)
FWIW - if it's easier to deal with it might be possible to modify the markup/css so that it looks more like: <button dir="auto" /> <div dir="auto"> <label> <span>${aValue}</span> <-- First half (when overflowing) <span><span>${aValue}</span></span> <-- Second half (when overflowing) ${aValue} <-- Full string (when not overflowing) </label> </div> So both the button and div would be direct children of the host element. The reason I currently have the outer div is so we can use flexbox to make the inner div stretch. There may be another way to accomplish that without introducing the container div.
(In reply to Brian Grinstead [:bgrins] from comment #17) > FWIW - if it's easier to deal with it might be possible to modify the > markup/css so that it looks more like: If that's possible that would be great. I can update the patch a bit to do an extra check.
Attachment #8968467 - Flags: review?(ehsan)
Attachment #8969232 - Flags: review?(ehsan)
Attachment #8969233 - Flags: review?(ehsan)
I think I've figured it out. Please read my proposed commit messages. Dealing with the execution order of procedural code on a declarative tree feels like a recurring theme...
Assignee: nobody → timdream
Status: NEW → ASSIGNED
I've manually confirmed the patch posted work with the proposed markup in comment 14. It works because ${aValue} (the file name) happens to be the same even if the dir=auto node is "complex". ``` <div> <button dir="auto"> <-- dir=auto node Browse... <-- direct children affect the directionality of parent </button> <div> <label dir="auto"> <-- dir=auto node <span>${aValue}</span> <-- does *not* affact the parent (but does inherit the directionality from parent) <span><span>${aValue}</span></span> <-- does *not* affact the parent (but does inherit the directionality from parent) ${aValue} <-- direct children affect the directionality of parent </label> </div> </div> ```
Just a random note, DirectionalityUtils has proved to be crazy crash-prone. So be exceptionally careful when modifying it. (I think we should somehow rewrite the whole DirectionalityUtils at some point, but I don't have too good ideas what the new implementation should look like.)
I agree. See comment 22. Hopefully, the proposed patch here, which tailor toward the exactly limited use cases we need in NAC, could prevent me from hitting these crashes.
Comment on attachment 8969232 [details] Bug 1451576 - Add dir=auto to subtitle menu items in video control https://reviewboard.mozilla.org/r/237968/#review245486 ::: commit-message-2857c:5 (Diff revision 4) > +Bug 1451576 - Add dir=auto to subtitle menu items in video control > + > +The labels of the subtitle track selections come from the web content, > +from <track label="...">. Given that it will likely be the name of the > +script in its native script, each of the item should have its own What's "script" here? Did you mean language? ::: commit-message-2857c:10 (Diff revision 4) > +script in its native script, each of the item should have its own > +directionality, instead of inheriting direction: ltr set on the entire > +<xul:videocontrol> parent element. > + > +I do however wonders if the web content should have the opportunity to > +affect the directionality of these labels? Yes, why wouldn't it? If the user is reading the page in their language, they're expecting it to show up with a sensible direction... I generally consider any place in our UI where we display a string from a web page without using dir=auto a bug.
Attachment #8969232 - Flags: review?(ehsan) → review+
Comment on attachment 8969233 [details] Bug 1451576 - Set dir=auto on [Browse...] button of <input type=file> https://reviewboard.mozilla.org/r/237970/#review245492
Attachment #8969233 - Flags: review?(ehsan) → review+
Comment on attachment 8968467 [details] Bug 1451576 - Allow dir=auto to work in native anonymous content https://reviewboard.mozilla.org/r/237160/#review245496 I think the limitation in comment 32 is probably OK for the current use cases we have here... r=me with that in mind. Thanks for the patch! ::: dom/base/DirectionalityUtils.cpp:285 (Diff revision 6) > inline static bool > NodeAffectsDirAutoAncestor(nsINode* aTextNode) > { > Element* parent = aTextNode->GetParentElement(); > + // The text node inside an anonymous subtree only affects > + // the direct dir=auto parent in the same anonymous subtree. This comment reads as if this is how things should work, but I think you meant to explain the limitation in our implementation. Do you mind rewording this please to make it read like we currently only support this case? ::: dom/base/DirectionalityUtils.cpp:935 (Diff revision 6) > > Directionality dir = GetDirectionFromText(aTextNode->GetText()); > - if (dir != eDir_NotSet && aTextNode->HasTextNodeDirectionalityMap()) { > + if (dir != eDir_NotSet) { > nsTextNodeDirectionalityMap::ResetTextNodeDirection(aTextNode, aTextNode); > + } else { > + nsTextNodeDirectionalityMap::EnsureMapIsClearFor(aTextNode); Can you please add a comment here like this? // We used to check NodeAffectsDirAutoAncestor() in this function, but // stopped doing that since calling IsInAnonymousSubtree() // too late (during nsTextNode::UnbindFromTree) is impossible and this // function was no-op when there's no directionality map. something along those lines.
Attachment #8968467 - Flags: review?(ehsan) → review+
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8e51a02b3d48 Allow dir=auto to work in native anonymous content r=Ehsan https://hg.mozilla.org/integration/autoland/rev/c576cede0ed1 Add dir=auto to subtitle menu items in video control r=Ehsan https://hg.mozilla.org/integration/autoland/rev/09f7885a83ce Set dir=auto on [Browse...] button of <input type=file> r=Ehsan
https://hg.mozilla.org/mozilla-central/rev/8e51a02b3d48 https://hg.mozilla.org/mozilla-central/rev/c576cede0ed1 https://hg.mozilla.org/mozilla-central/rev/09f7885a83ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This should've landed with tests... Also, this causes invariant violations like bug 1467964. You can't just change ancestors' direction from Layout, because it affects the style of the element, which can affect the layout itself.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/194ef402a7b1 Revert three changesets for causing bug 1467964 and since it's not generally sound. r=me
https://hg.mozilla.org/mozilla-central/rev/194ef402a7b1 Does this need backing out from Beta too then?
Status: RESOLVED → REOPENED
status-firefox62: --- → affected
Flags: needinfo?(emilio)
Resolution: FIXED → ---
I don't think it's a safety issue as things stand, only a correctness issue, so I talked with Tim yesterday and we're fine not backing this out from beta I think.
Flags: needinfo?(emilio)
This bug should be closed given there is no further action needed. Not sure I should set this to WONTFIX/INVALID or FIXED. Feel free to correct it.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox62: affected → wontfix
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: