Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302433015:
Use correct offset when attribute auto is present (Closed)
|
Created:
6 years, 6 months ago by Habib Virji Modified:
6 years, 6 months ago Reviewers:
leviw_travelin_and_unemployed, dglazkov, tkent, mario.prada, esprehn, fs, eseidel, Inactive CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUse correct offset when attribute auto is present
Depending on the inlineBox bidiLevel use the right or left offset.
Do not use bidiLevel of the previous box when attribute is auto.
R=eseidel, leviw
BUG=296847
TEST=TextArea with strong RTL word, when on next line LTR word is
entered, caret position is changed.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176151
Patch Set 1 #Patch Set 2 : Updated to latest master #
Total comments: 11
Patch Set 3 : Updated as per code review comments #
Total comments: 3
Patch Set 4 : Added a check for shadow node, to check attribute direction #Patch Set 5 : Updated to latest master #
Total comments: 4
Patch Set 6 : Rename isTextDirectionHasAutoAttribute to hasDirectionAutoAttribute #
Total comments: 4
Patch Set 7 : Position trying to look in multiple level for direction auto #
Total comments: 11
Patch Set 8 : Changed traversing of node from downwards to upwards #
Total comments: 13
Patch Set 9 : Updated code as per mario's comments #Patch Set 10 : Updated tests to pass on mac and windows. Also a condition in HTMLElement which sets any direction … #
Total comments: 2
Patch Set 11 : Updated setHasDirAutoFlagRecursively to set shadowDOM node #Patch Set 12 : Removes hasDirectionAutoAttribute function and sets all nodes in setHasDirAutoFlagRecursively with … #
Total comments: 2
Patch Set 13 : Updated to not just not just user agent shadow root #Patch Set 14 : Added test for author shadow roots #
Total comments: 2
Patch Set 15 : Fix compilation issue #
Messages
Total messages: 55 (0 generated)
When attribute is auto and text entered at first has dir rtl, then entered text if is of direction ltr, getInlineBoxAndOffset does not set caret position, leaving caret in the start of the line at postion 0. Since text area can have different dir text, this patch facilitates moving dir in the text area and using the bidiLevel of the inlineBox selected. Reason for this change is before entering the condition inlineBox bidiLevel is checked, but after selecting new inlineBox, bidiLevel is not checked, leading to the caret getting set of previous inline box. With the patch, caret is position correctly after hello!. https://chromium.googlecode.com/issues/attachment?aid=2968470008000&name=care...
https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... LayoutTests/editing/selection/caret-in-textarea-auto.html:16: caretRect = textInputController.firstRectForCharacterRange(1, 0); Can we test this programmatically instead of using a render tree dump? Why are you getting the caretRect but not using it? https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1263: // A special scenario when attribute auto is set. if the direction is changed use right offset It's not just "attribute auto," but dirAuto. Instead of the comment, how about a static method with an informative name? This function is already way too long. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1265: if (deprecatedNode()->document().focusedElement()) Why the check for a focused element? https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1266: toHTMLElement(deprecatedNode()->document().focusedElement())->directionalityIfhasDirAutoAttribute(isAuto); directionalityIfhasDirAutoAttributre returns the directionality, which you're not using here. You're also calling deprecatedNode() to get the document. You should use the document method or poke directly at m_anchorNode since you're inside of position code.
Looks like leviw's review was a not l-g-t-m.
Thanks Levi will update the patch with suggested comments soon. https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... LayoutTests/editing/selection/caret-in-textarea-auto.html:16: caretRect = textInputController.firstRectForCharacterRange(1, 0); On 2014/05/28 17:42:31, leviw wrote: > Can we test this programmatically instead of using a render tree dump? Why are > you getting the caretRect but not using it? A valid point will correct it. Was planning to use it but instead thought of using render tree as it was providing caret information. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1263: // A special scenario when attribute auto is set. if the direction is changed use right offset On 2014/05/28 17:42:31, leviw wrote: > It's not just "attribute auto," but dirAuto. > > Instead of the comment, how about a static method with an informative name? This > function is already way too long. Ok thanks will update accordingly. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1265: if (deprecatedNode()->document().focusedElement()) On 2014/05/28 17:42:31, leviw wrote: > Why the check for a focused element? I need a way to access directionality function and wanted to make sure element being checked for attribute has direction auto set. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1266: toHTMLElement(deprecatedNode()->document().focusedElement())->directionalityIfhasDirAutoAttribute(isAuto); On 2014/05/28 17:42:31, leviw wrote: > directionalityIfhasDirAutoAttributre returns the directionality, which you're > not using here. > > You're also calling deprecatedNode() to get the document. You should use the > document method or poke directly at m_anchorNode since you're inside of position > code. directionalityIfhasDirAutoAttribute takes bool argument and returns true/false too depending on if the attribute is present. I am not using TextDirection as bidiLevel checks were in place. Okay will try with m_anchorNode was not aware about it. Thanks.
Thanks Levi, addressed most of your comments except focusedElement issue. If you are refering to using node's selfOrAncestorHasDirAutoAttribute(), then the issue is does not return if attribute is set to the element but using document's focusedElement i was able to find element having a attribtue auto set. https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): https://codereview.chromium.org/302433015/diff/10001/LayoutTests/editing/sele... LayoutTests/editing/selection/caret-in-textarea-auto.html:16: caretRect = textInputController.firstRectForCharacterRange(1, 0); On 2014/05/28 17:42:31, leviw wrote: > Can we test this programmatically instead of using a render tree dump? Why are > you getting the caretRect but not using it? Done. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1263: // A special scenario when attribute auto is set. if the direction is changed use right offset On 2014/05/28 17:42:31, leviw wrote: > It's not just "attribute auto," but dirAuto. > > Instead of the comment, how about a static method with an informative name? This > function is already way too long. Done. https://codereview.chromium.org/302433015/diff/10001/Source/core/dom/Position... Source/core/dom/Position.cpp:1266: toHTMLElement(deprecatedNode()->document().focusedElement())->directionalityIfhasDirAutoAttribute(isAuto); On 2014/05/28 17:42:31, leviw wrote: > directionalityIfhasDirAutoAttributre returns the directionality, which you're > not using here. > > You're also calling deprecatedNode() to get the document. You should use the > document method or poke directly at m_anchorNode since you're inside of position > code. Done.
https://codereview.chromium.org/302433015/diff/30005/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/30005/Source/core/dom/Position... Source/core/dom/Position.cpp:82: static bool isTextDirectionAuto(Node *node) This is asking to be misused. This function doesn't answer the question of whether or not the node you pass in is dirAuto... It's answering whether or not the focused element is. https://codereview.chromium.org/302433015/diff/30005/Source/core/dom/Position... Source/core/dom/Position.cpp:85: if (node->document().focusedElement()) This just isn't right. getInlineBoxAndOffset shouldn't give different answers depending on the focused node. I'm fine with you crawling up the ancestor chain for node, which in your test should yield you the node you're using here, right?
https://codereview.chromium.org/302433015/diff/30005/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/30005/Source/core/dom/Position... Source/core/dom/Position.cpp:85: if (node->document().focusedElement()) On 2014/05/29 16:32:18, leviw wrote: @leviw: An update, still struggling: The text area typically renders into: RenderTextControl {TEXTAREA}, RenderBlock {DIV} RenderText {#text} So when i traverse upward from anchorNode in getInlineBoxAnfOffset, i end up in Div and not in TextArea. (I am looking for if node is isBlock or isElement node.). Attribute dir does not seem to be set for div. Regarding selfOrAncestorHasDirAutoAttribute, which gets set from HtmlElement does not return for the focus node instead when the element is not in focus it returns correct result. I am debugging further to understand this behavior.
Looking at that code, it's possible that setHasDirAutoFlagRecursively doesn't descend into a TextArea properly (due to a shadow boundary). Adding esprehn who knows more about shadow roots.
On 2014/05/30 18:54:55, leviw wrote: > Looking at that code, it's possible that setHasDirAutoFlagRecursively doesn't > descend into a TextArea properly (due to a shadow boundary). > > Adding esprehn who knows more about shadow roots. @leviw: I have added a check for shadow node if it has dir attribute in isTextDirectionHasAutoAttribute. Same check does not get applied in setHasDirAutoFlagRecursively, as it does not seem to have shadow node information.
https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... Source/core/dom/Position.cpp:82: static bool isTextDirectionHasAutoAttribute(Node *node) nit: star on wrong side. nit: "isTextDirectionHasAutoAttribute" does not sound correct. We should probably find a better name? https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... Source/core/dom/Position.cpp:85: if (node->isInShadowTree()) { nit: No need for curly brackets.
Thanks chris for reviewing. Updating code review comments. Has renamed isTextDirectionHasAutoAttribute to hasDirectionAutoAttribute. Is it okay? https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... Source/core/dom/Position.cpp:82: static bool isTextDirectionHasAutoAttribute(Node *node) On 2014/06/02 15:15:08, Chris Dumez wrote: > nit: star on wrong side. > nit: "isTextDirectionHasAutoAttribute" does not sound correct. We should > probably find a better name? Done. https://codereview.chromium.org/302433015/diff/70001/Source/core/dom/Position... Source/core/dom/Position.cpp:85: if (node->isInShadowTree()) { On 2014/06/02 15:15:08, Chris Dumez wrote: > nit: No need for curly brackets. Done.
Perhaps dglazkov can comment or point this towards someone who can speak to the right behavior for shadow DOM + dir=auto. https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... Source/core/dom/Position.cpp:86: isAuto = equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto"); This seems specific to TextArea. If you're inside arbitrary Shadow DOM content, you may not want this. At a minimum, it seems like if isAuto is true before this line, the lack of this attribute on the shadowHost shouldn't blow it away, so maybe you want if (!isAuto node->isInShadowTree())? Or perhaps we should already be propagating the selfOrAncestorHasDirAutoAttribute across shadow boundaries? I'd love to hear from some of our shadow DOM experts.
This doesn't seem right to me, you only look up one level. Either we should make this property inherit correctly or we should make it use the distributed parent. That doesn't actually fix <textarea> though since it's not really using distribution. https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... Source/core/dom/Position.cpp:86: isAuto = equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto"); I don't think this is correct, what if you're multiple levels down?
https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... Source/core/dom/Position.cpp:86: isAuto = equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto"); @levi: If I get some assistance about how to find information about shadow elements in setHasDirAutoFlagRecursively, it will help in setting correct values for selfOrAncestorHasDirAutoAttribute. I have upload a new patch which tries to return early to address above issue, instead of checking shadowtree node. https://codereview.chromium.org/302433015/diff/90001/Source/core/dom/Position... Source/core/dom/Position.cpp:86: isAuto = equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto"); On 2014/06/02 18:06:13, esprehn wrote: > I don't think this is correct, what if you're multiple levels down? @esprehn: i have upload a new patch which check multiple down, if dir auto attribute is set. It is similar to approach in setHasDirAutoFlagRecursively but instead looking for shadow nodes.
https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:88: if (node->isInShadowTree()) { If you're scoping your NodeTraversal to your anchorNode, this shouldn't need to be in the loop. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: if (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) is fastGetAttribute(dirAttr) == "auto" ever true when selfOrAncestorHasDirAutoAttribute() isn't? https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:92: node = NodeTraversal::next(*node, anchorNode); Why are you using anchorNode as your container? This loop doesn't make a lot of sense to me.
This still doesn't look right, you need to make the selfOrAncestor bits work across shadow roots, you shouldn't be checking shadowRoot(). https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... LayoutTests/editing/selection/caret-in-textarea-auto.html:3: <body> We usually leave off the <html>, <head> and <body> tags.
On 2014/06/04 at 18:02:02, esprehn wrote: > This still doesn't look right, you need to make the selfOrAncestor bits work across shadow roots, you shouldn't be checking shadowRoot(). > Err I meant you shouldn't be checking shadowHost().
On 2014/06/04 18:02:02, esprehn wrote: > This still doesn't look right, you need to make the selfOrAncestor bits work > across shadow roots, you shouldn't be checking shadowRoot(). > > https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... > File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): > > https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... > LayoutTests/editing/selection/caret-in-textarea-auto.html:3: <body> > We usually leave off the <html>, <head> and <body> tags. @esphern: Thanks for reviewing again. The issue as far as I understand is selfOrAncestor is set via HTMLElement and there accessing ElementShadow is not working. (I am trying to find if it is shadowNode and then trying to find oldestNode, but it fails to compile and shows ElementShadow is incomplete). Position.cpp is working on the shadow nodes, but selfOrAncestor are set on element node. Hence I have been using shadowHost to access element attributes. If not shadowHost is there any other way to access those attributes? Node structure in position appears to be Div document fragment text And in HTML Element TextArea Text. So from text node in position.cpp, I am trying to access shadowHost to get attributes.
On 2014/06/04 18:02:02, esprehn wrote: > This still doesn't look right, you need to make the selfOrAncestor bits work > across shadow roots, you shouldn't be checking shadowRoot(). > > https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... > File LayoutTests/editing/selection/caret-in-textarea-auto.html (right): > > https://codereview.chromium.org/302433015/diff/110001/LayoutTests/editing/sel... > LayoutTests/editing/selection/caret-in-textarea-auto.html:3: <body> > We usually leave off the <html>, <head> and <body> tags. @esphern: Thanks for reviewing again. The issue as far as I understand is selfOrAncestor is set via HTMLElement and there accessing ElementShadow is not working. (I am trying to find if it is shadowNode and then trying to find oldestNode, but it fails to compile and shows ElementShadow is incomplete). Position.cpp is working on the shadow nodes, but selfOrAncestor are set on element node. Hence I have been using shadowHost to access element attributes. If not shadowHost is there any other way to access those attributes? Node structure in position appears to be Div document fragment text And in HTML Element TextArea Text. So from text node in position.cpp, I am trying to access shadowHost to get attributes.
https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:88: if (node->isInShadowTree()) { On 2014/06/04 17:16:32, leviw wrote: > If you're scoping your NodeTraversal to your anchorNode, this shouldn't need to > be in the loop. Ok thanks, i have corrected it. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: if (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) On 2014/06/04 17:16:32, leviw wrote: > is fastGetAttribute(dirAttr) == "auto" ever true when > selfOrAncestorHasDirAutoAttribute() isn't? @leviw: Yes it does work and satisfy the bug test case. It appears position works with shadowDOM nodes and to access attribute i have to use shadowHost attributes. While setFlagOrAncestor seem to work on nonShadowElement. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:92: node = NodeTraversal::next(*node, anchorNode); On 2014/06/04 17:16:32, leviw wrote: > Why are you using anchorNode as your container? This loop doesn't make a lot of > sense to me. @leviw: i was trying to address, esphern comment about trying to look more than 1 level for shadowHost.
https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: if (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) On 2014/06/05 18:57:53, Habib Virji wrote: > On 2014/06/04 17:16:32, leviw wrote: > > is fastGetAttribute(dirAttr) == "auto" ever true when > > selfOrAncestorHasDirAutoAttribute() isn't? > @leviw: Yes it does work and satisfy the bug test case. It appears position > works with shadowDOM nodes and to access attribute i have to use shadowHost > attributes. While setFlagOrAncestor seem to work on nonShadowElement. If while walking you come across an ancestor with dir=LTR, you'll just keep walking with this code until you happen to find one with dir=auto. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:92: node = NodeTraversal::next(*node, anchorNode); On 2014/06/05 18:57:53, Habib Virji wrote: > On 2014/06/04 17:16:32, leviw wrote: > > Why are you using anchorNode as your container? This loop doesn't make a lot > of > > sense to me. > > @leviw: i was trying to address, esphern comment about trying to look more than > 1 level for shadowHost. I understand what you wanted to address, but this isn't the right way to do that. Higher shadow hosts won't be next in the NodeTraversal. You're walking across when he was saying you have to walk up.
Thanks levi. Updated code to move upwards towards parent and look for shadowRoot. Once shadowRoot is found using shadowHost to find if direction attribute is present. If dir is auto return true. To answer esphern suggestion to setFlagOrAncestor: 1. Though it is right solution it just returns the flag set by setHasDirAutoFlagRecursively. 2. setHasDirAutoFlag, traverses node downwards but to find shadowRoot it will require to go in upwards direction. 3. Also in HTMLElement if shadowRoot is found how attribute dir should be set is not clear. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: if (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) Done. Thanks, updated code to check if direction is present. If dir auto then return true else false. https://codereview.chromium.org/302433015/diff/110001/Source/core/dom/Positio... Source/core/dom/Position.cpp:92: node = NodeTraversal::next(*node, anchorNode); Done. Corrected now.
https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:86: return true; I might be entirely wrong, but I think it would be better if this if was placed before the while, and that you only iterated walking up the tree if node->isInShadowTree(), until you find the shadowRoot (to ask for the shadow host). https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: return (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) ? true: false; Would it be possible to replace this two last lines with node->shadowHost()->selfOrAncestorHasDirAutoAttribute? https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1251: if (!prevBox || (prevBox && (prevBox->bidiLevel() < level))) { The (!prevBox || prevBox->bidiLevel() < level) is enough, as the OR is evaluated in short-cirtuit https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1259: } else if (prevBox && (prevBox->bidiLevel() > level)) { You don't need the "prevBox &&" part here either (you will only reach this if prevBox is non-null) https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1270: if (!nextBox || (nextBox && nextBox->bidiLevel() < level)) { Same comments here https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1281: } else if (nextBox && (nextBox->bidiLevel() > level)) { (nextBox->bidiLevel() > level) is enough here too
Thanks Mario... Updated code as per your comments. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:86: return true; On 2014/06/06 14:34:51, mario.prada wrote: > I might be entirely wrong, but I think it would be better if this if was placed > before the while, and that you only iterated walking up the tree if > node->isInShadowTree(), until you find the shadowRoot (to ask for the shadow > host). Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:86: return true; On 2014/06/06 14:34:51, mario.prada wrote: > I might be entirely wrong, but I think it would be better if this if was placed > before the while, and that you only iterated walking up the tree if > node->isInShadowTree(), until you find the shadowRoot (to ask for the shadow > host). Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: return (equalIgnoringCase(node->shadowHost()->fastGetAttribute(dirAttr), "auto")) ? true: false; On 2014/06/06 14:34:51, mario.prada wrote: > Would it be possible to replace this two last lines with > node->shadowHost()->selfOrAncestorHasDirAutoAttribute? Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1251: if (!prevBox || (prevBox && (prevBox->bidiLevel() < level))) { On 2014/06/06 14:34:52, mario.prada wrote: > The (!prevBox || prevBox->bidiLevel() < level) is enough, as the OR is evaluated > in short-cirtuit Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1259: } else if (prevBox && (prevBox->bidiLevel() > level)) { On 2014/06/06 14:34:52, mario.prada wrote: > You don't need the "prevBox &&" part here either (you will only reach this if > prevBox is non-null) Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1270: if (!nextBox || (nextBox && nextBox->bidiLevel() < level)) { On 2014/06/06 14:34:52, mario.prada wrote: > Same comments here Done. https://codereview.chromium.org/302433015/diff/130001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1281: } else if (nextBox && (nextBox->bidiLevel() > level)) { On 2014/06/06 14:34:52, mario.prada wrote: > (nextBox->bidiLevel() > level) is enough here too Done.
Patch#10, makes a small change in HTMLElement as any direction was setting auto flag. Changed to set flag only when node has attribute auto.
On 2014/06/06 17:02:59, Habib Virji wrote: > Patch#10, makes a small change in HTMLElement as any direction was setting auto > flag. Changed to set flag only when node has attribute auto. @leviw hi, sorry for trouble. Any better.
This is still not right, the issue is that setHasDirAutoFlagRecursively doesn't cross ShadowRoot boundaries. https://codereview.chromium.org/302433015/diff/170001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/170001/Source/core/dom/Positio... Source/core/dom/Position.cpp:88: if (node->isShadowRoot()) This can just be node->containingShadowRoot() which is constant time and has no tree walk.
Thanks esprehn for looking. I have updated to use containingShadowRoot. Also added a change in HTMLElement(setDirHas..) to check if node is in shadowTree then update shadowHost to have dir auto flag. https://codereview.chromium.org/302433015/diff/170001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/170001/Source/core/dom/Positio... Source/core/dom/Position.cpp:88: if (node->isShadowRoot()) On 2014/06/11 00:58:01, esprehn wrote: > This can just be node->containingShadowRoot() which is constant time and has no > tree walk. Done.
Hi Habib, see some comments below, but please don't take them as a proper review, but as some comments trying to help with this patch. If anything I'm saying is no sense, please let me know :) https://codereview.chromium.org/302433015/diff/190001/Source/core/dom/Positio... File Source/core/dom/Position.cpp (right): https://codereview.chromium.org/302433015/diff/190001/Source/core/dom/Positio... Source/core/dom/Position.cpp:89: return node->shadowHost()->selfOrAncestorHasDirAutoAttribute(); shadowHost() already calls containingShadowRoot(), and this other function already returns you the root node from any point in the shadow tree, so you should not need this while loop here, just call node->shadowHost() (and probably check whether it returns a valid node or null) https://codereview.chromium.org/302433015/diff/190001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1277: if (hasDirectionAutoAttribute(m_anchorNode.get())) After reading esprehn's comments again, my feeling is that you don't need to create this new function hasDirectionAutoAttribute() but fix setHasDirAutoFlagRecursively() to be able to cross the shadow boundaries and then just use selfOrAncestorHasDirAutoAttribute() here. https://codereview.chromium.org/302433015/diff/190001/Source/core/dom/Positio... Source/core/dom/Position.cpp:1278: caretOffset = (inlineBox->bidiLevel() < level) ? inlineBox->caretLeftmostOffset() : inlineBox->caretRightmostOffset(); Style nit: you don't need the () around the condition :) https://codereview.chromium.org/302433015/diff/190001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/302433015/diff/190001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:637: if (firstNode->isInShadowTree()) { This block walks up the tree from inside the shadow tree until it reaches the shadow host, but I believe the point is precisely to do it the other way around: to cross the shadow boundaries while going top-down (in the while loop after this point) What is not clear to me, though, is whether calling containingShadowRoot() over the host will give you the shadow root or not. I'm afraid you'll need to investigate that :)
@esprehn/@mario: Thanks for the review have uploaded a new patch to set shadowRoot with dir flag using setHasDirAutoFlagRecursively. In HTMLElement, looking for userAgentShadowRoot() (which includes containingShadowRoot() and shadow() call) and then passing shadowRoot to sets the flag for the node and its children. (Though in setHasDirAutoFlagRecursively, it checks if node or ancestor has flag before traversing to next node, since it will be true as first lines sets the firstChild to true it returns back, so child nodes does not get set, not sure if it is intended or a bug). In position.cpp, function is simplified to traverse till parent as setHasDirAutoFlagRecursively does not set all child, only parent nodes are set.
@esprehn/mario: Have uploaded two patches. Patch#11 and Patch#12 - Both patch sets shadowRoot with direction auto flag using setHasDirAutoFlagRecursively. - Both patch in HTMLElement, looks for userAgentShadowRoot() and then pass shadowRoot to set the flag for the node and its children. Patch #11 includes in position.cpp, hasDirectionAutoAttirbute function to traverse till parent as setHasDirAutoFlagRecursively does not set all child, only parent nodes are set. (setHasDirAutoFlagRecursively, it checks if node or ancestor has flag before traversing to next node, since it will be true as first line sets the firstChild to true it returns back, so child nodes does not get set). Patch #12: Corrects setHasDirAutoFlagRecursively and set all child nodes with the direction flag. It removes the function in position.cpp (hasDirectionAutoAttribute) as selfOrAncestorHasDirAttribute returns correct value for shadow node and its children too.
This behavior shouldn't be specific to UA shadows. https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:757: setHasDirAutoFlagRecursively(userAgentShadowRoot(), true); This should be doing: for(ShadowRoot* root = youngestShadowRoot(); root; root = root->olderShadowRoot()) setHasDirAutoFlagRecursively(root, true);
Thanks. Updated as suggested. https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:757: setHasDirAutoFlagRecursively(userAgentShadowRoot(), true); On 2014/06/11 17:38:07, esprehn wrote: > This should be doing: > > for(ShadowRoot* root = youngestShadowRoot(); root; root = > root->olderShadowRoot()) > setHasDirAutoFlagRecursively(root, true); Done.
On 2014/06/11 at 17:48:11, habib.virji wrote: > Thanks. Updated as suggested. > > https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... > File Source/core/html/HTMLElement.cpp (right): > > https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... > Source/core/html/HTMLElement.cpp:757: setHasDirAutoFlagRecursively(userAgentShadowRoot(), true); > On 2014/06/11 17:38:07, esprehn wrote: > > This should be doing: > > > > for(ShadowRoot* root = youngestShadowRoot(); root; root = > > root->olderShadowRoot()) > > setHasDirAutoFlagRecursively(root, true); > > Done. Please add tests for author shadow roots too. You can do that with createShadowRoot() and some directional text inside it.
On 2014/06/11 18:11:50, esprehn wrote: > On 2014/06/11 at 17:48:11, habib.virji wrote: > > Thanks. Updated as suggested. > > > > > https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... > > File Source/core/html/HTMLElement.cpp (right): > > > > > https://codereview.chromium.org/302433015/diff/230001/Source/core/html/HTMLEl... > > Source/core/html/HTMLElement.cpp:757: > setHasDirAutoFlagRecursively(userAgentShadowRoot(), true); > > On 2014/06/11 17:38:07, esprehn wrote: > > > This should be doing: > > > > > > for(ShadowRoot* root = youngestShadowRoot(); root; root = > > > root->olderShadowRoot()) > > > setHasDirAutoFlagRecursively(root, true); > > > > Done. > > Please add tests for author shadow roots too. You can do that with > createShadowRoot() and some directional text inside it. Have added a test, it checks if shadowRoot direction of the parent node.
This seems okay, but looking at the code the dir=auto logic is _very_ broken. For example adjustDirectionalityIfNeededAfterChildrenChanged isn't called when you add a ShadowRoot, so if you set dir=auto, then add a ShadowRoot I don't think it'll inherit properly. Also if you remove the dir=auto attribute it won't correctly update the bit in that subtree. If leviw@ is okay with this patch I think we can land it, but the dir=auto stuff is so terribly broken I'm not sure we should really be adding more stuff. https://codereview.chromium.org/302433015/diff/270001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/302433015/diff/270001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:640: if (node->selfOrAncestorHasDirAutoAttribute() == flag) This means we're going to walk down the subtree even if all the bits are the same now?
Agreed. Will open a new CL with some possible fixes to correct dir=auto. Since this issue was to fix caret offset and it needed dir=auto information and this CL appears is prviding. https://codereview.chromium.org/302433015/diff/270001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/302433015/diff/270001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:640: if (node->selfOrAncestorHasDirAutoAttribute() == flag) Yes that's true. Reason was it was preventing child nodes, to be set and was returning early.
Okay. LGTM. Please file a bug for the brokenness esprehn mentioned. esprehn: thanks for the help with the ShadowDOM parts.
On 2014/06/12 18:02:10, leviw wrote: > Okay. LGTM. Please file a bug for the brokenness esprehn mentioned. > > esprehn: thanks for the help with the ShadowDOM parts. Thanks leviw and esprehn for all your help. @leviw: Will add a bug about dir=auto brokeness.
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/302433015/270001
The CQ bit was unchecked by habib.virji@samsung.com
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/302433015/330001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/302433015/330001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/302433015/330001
Message was sent while issue was closed.
Change committed as 176151 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
