[a11y] Simplify list marker navigation around ignored content [chromium/src : main]
Jacques Newman (Gerrit)
Jacques Newman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
2 comments
I love changes that are negative in lines of code 😊
Change mostly lgtm with one concern, as mentioned in the comment, I really am a little on the fence on how we move forward here, but want to make sure we do so carefully.
<< "We don't support finding unignored siblings for ignored "
"objects because it is not clear whether to search for the "
"sibling in the unignored tree or in the whole tree: "For this and the other methods:
I've gone back and forth on my preference a few times, but this message still resonates with me, and we are explicitly breaking it here by allowing the method to be called by an ignored object that is not included in the tree.
Option 1)
Update the error message to be explicit that this is something we actually support, or maybe remove it altogether if this is a behavior we do want to allow. I am little hesitant here but I understand the desire to have this functionality.
Option 2)
I keep thinking what you really want here is something like "AsUnignoredNextSiblingSlow" that is meant to be called on an ignored node to get the closest next sibling, but I don't like the redundant logic that brings with it.
Related details
- Kevin Ellis
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucas Radaelli (Gerrit)
Lucas Radaelli added 1 comment![]( "Open in Gerrit")
<< "We don't support finding unignored siblings for ignored "
"objects because it is not clear whether to search for the "
"sibling in the unignored tree or in the whole tree: "For this and the other methods:
I've gone back and forth on my preference a few times, but this message still resonates with me, and we are explicitly breaking it here by allowing the method to be called by an ignored object that is not included in the tree.
Option 1)
Update the error message to be explicit that this is something we actually support, or maybe remove it altogether if this is a behavior we do want to allow. I am little hesitant here but I understand the desire to have this functionality.Option 2)
I keep thinking what you really want here is something like "AsUnignoredNextSiblingSlow" that is meant to be called on an ignored node to get the closest next sibling, but I don't like the redundant logic that brings with it.
I disagree that we are calling the method on an ignored object. What is happening there is that we are allowing this method to be called on Ignored (but included!), in tree, which is different.
I agree this method is not clear if it should work at all for ture, ignored objects, but ignored but included objects are different, they exist to make the tree stitching correct, but we don't serialize most of its attributes because they would be redundant. This is the case of the text object that is a child of the list marker, which is the only ignored but included in tree node would call this method.
Related details
- Jacques Newman
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman added 1 comment![]( "Open in Gerrit")
<< "We don't support finding unignored siblings for ignored "
"objects because it is not clear whether to search for the "
"sibling in the unignored tree or in the whole tree: "Lucas RadaelliFor this and the other methods:
I've gone back and forth on my preference a few times, but this message still resonates with me, and we are explicitly breaking it here by allowing the method to be called by an ignored object that is not included in the tree.
Option 1)
Update the error message to be explicit that this is something we actually support, or maybe remove it altogether if this is a behavior we do want to allow. I am little hesitant here but I understand the desire to have this functionality.Option 2)
I keep thinking what you really want here is something like "AsUnignoredNextSiblingSlow" that is meant to be called on an ignored node to get the closest next sibling, but I don't like the redundant logic that brings with it.
I disagree that we are calling the method on an ignored object. What is happening there is that we are allowing this method to be called on Ignored (but included!), in tree, which is different.
I agree this method is not clear if it should work at all for ture, ignored objects, but ignored but included objects are different, they exist to make the tree stitching correct, but we don't serialize most of its attributes because they would be redundant. This is the case of the text object that is a child of the list marker, which is the only ignored but included in tree node would call this method.
Even if this ignored object is included in the tree, it doesn't mean that it isn't ignored, so I don't think we can say that we aren't calling this on an ignored object.
I would be OK with rewriting the DUMP_WILL_BE_NOTREACHED message to replace "ignored" with "included" so it matches the new behavior.
Related details
- Kevin Ellis
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucas Radaelli (Gerrit)
Lucas Radaelli added 1 comment![]( "Open in Gerrit")
<< "We don't support finding unignored siblings for ignored "
"objects because it is not clear whether to search for the "
"sibling in the unignored tree or in the whole tree: "Lucas RadaelliFor this and the other methods:
I've gone back and forth on my preference a few times, but this message still resonates with me, and we are explicitly breaking it here by allowing the method to be called by an ignored object that is not included in the tree.
Option 1)
Update the error message to be explicit that this is something we actually support, or maybe remove it altogether if this is a behavior we do want to allow. I am little hesitant here but I understand the desire to have this functionality.Option 2)
I keep thinking what you really want here is something like "AsUnignoredNextSiblingSlow" that is meant to be called on an ignored node to get the closest next sibling, but I don't like the redundant logic that brings with it.
Jacques NewmanI disagree that we are calling the method on an ignored object. What is happening there is that we are allowing this method to be called on Ignored (but included!), in tree, which is different.
I agree this method is not clear if it should work at all for ture, ignored objects, but ignored but included objects are different, they exist to make the tree stitching correct, but we don't serialize most of its attributes because they would be redundant. This is the case of the text object that is a child of the list marker, which is the only ignored but included in tree node would call this method.
Even if this ignored object is included in the tree, it doesn't mean that it isn't ignored, so I don't think we can say that we aren't calling this on an ignored object.
I would be OK with rewriting the DUMP_WILL_BE_NOTREACHED message to replace "ignored" with "included" so it matches the new behavior.
Done
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |