Closed Bug 1015550 Opened 11 years ago Closed 10 years ago

Line boundary is incorrect for multi-char bullet

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file, 4 obsolete files)

patch, r=surkov
7.30 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
Line boundary is incorrect for bullets have more than one character.
Attached patch patch (obsolete) — — Splinter Review
Attachment #8428210 - Flags: review?(surkov.alexander)
Attached patch patch (obsolete) — — Splinter Review
Attachment #8428210 - Attachment is obsolete: true
Attachment #8428210 - Flags: review?(surkov.alexander)
Attachment #8428448 - Flags: review?(surkov.alexander)
Comment on attachment 8428448 [details] [diff] [review] patch Review of attachment 8428448 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/generic/HyperTextAccessible.cpp @@ +449,5 @@ > if (child == li->Bullet()) { > + if (text != this) { > + return aDirection == eDirPrevious ? > + TransformOffset(text, 0, false) : > + TransformOffset(text, 1, true); say list bullet is 2 chars, offset is 2, move by chars, eDirPrevious then offset is 1 which is not 0 offset relative parents. It doesn't mean you have to fix it but you need to give proper commenting @@ +452,5 @@ > + TransformOffset(text, 0, false) : > + TransformOffset(text, 1, true); > + } > + if (aDirection == eDirPrevious) { > + return 0; same here, btw a11y doesn't use {} around single ifs @@ +458,2 @@ > > + uint32_t nextOffset = GetChildOffset(1); nit: good to define it where needed @@ +458,4 @@ > > + uint32_t nextOffset = GetChildOffset(1); > + switch (aAmount) { > + case eSelectLine: nit: case should be 2 space indented @@ +458,5 @@ > > + uint32_t nextOffset = GetChildOffset(1); > + switch (aAmount) { > + case eSelectLine: > + case eSelectBeginLine: why beginLine here? @@ +464,3 @@ > // Ask a text leaf next (if not empty) to the bullet for an offset > // since list item may be multiline. > + return nextOffset < CharacterCount() ? btw, it doesn't work when line is a list bullet only wouldn't it nicer to compare aOffset and nextOffset instead? @@ +529,5 @@ > > // PeekOffset stops right before bullet so return 0 to workaround it. > + if (IsHTMLListItem() && aAmount == eSelectBeginLine && > + hyperTextOffset > 0) { > + Accessible* prevSibling = GetChildAtOffset(hyperTextOffset - 1); nit: prevOffsetChild?
(In reply to alexander :surkov from comment #4) > Comment on attachment 8428448 [details] [diff] [review] > patch > > Review of attachment 8428448 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/generic/HyperTextAccessible.cpp > @@ +458,2 @@ > > > > + uint32_t nextOffset = GetChildOffset(1); > > nit: good to define it where needed Since both branch in the switch use this variable, I think it is the proper place to define it. > @@ +458,5 @@ > > > > + uint32_t nextOffset = GetChildOffset(1); > > + switch (aAmount) { > > + case eSelectLine: > > + case eSelectBeginLine: > > why beginLine here? Is it guaranteed that eSelectBeginLine will only be passed in with eDirPrevious? If so, I will remove it. > @@ +464,3 @@ > > // Ask a text leaf next (if not empty) to the bullet for an offset > > // since list item may be multiline. > > + return nextOffset < CharacterCount() ? > > btw, it doesn't work when line is a list bullet only > > wouldn't it nicer to compare aOffset and nextOffset instead? It does work. This patch itself is able to pass test_lineboundary.html. The only problem I care about is that there will be an infinite recursion if length of bullet is zero. I didn't find evidence showed it would not, but I found a lot of code might be broken if an element has no character. Hence, I suppose that the bullet will never be empty.
Attached patch patch (obsolete) — — Splinter Review
Attachment #8428448 - Attachment is obsolete: true
Attachment #8428448 - Flags: review?(surkov.alexander)
Attachment #8429216 - Flags: review?(surkov.alexander)
(In reply to Xidorn Quan from comment #5) > > > + uint32_t nextOffset = GetChildOffset(1); > > > > nit: good to define it where needed > > Since both branch in the switch use this variable, I think it is the proper > place to define it. right but you still call the method for all other values not processed by switch statement > > > + case eSelectBeginLine: > > > > why beginLine here? > > Is it guaranteed that eSelectBeginLine will only be passed in with > eDirPrevious? If so, I will remove it. I see what you say, iirc the current logic doesn't allow this case but I agree it's better to have an assertion. > > @@ +464,3 @@ > > > // Ask a text leaf next (if not empty) to the bullet for an offset > > > // since list item may be multiline. > > > + return nextOffset < CharacterCount() ? > > > > btw, it doesn't work when line is a list bullet only > > > > wouldn't it nicer to compare aOffset and nextOffset instead? > > It does work. This patch itself is able to pass test_lineboundary.html. I don't see a proper test case in the mochitest. Say you have an example <li><br>hello</li> line is ending at nextOffset, nextOffset is lesser than CharacterCount and you call FindOffset for 'hello' text node what returns end of 'hello'. Does it sound correct? > The > only problem I care about is that there will be an infinite recursion if > length of bullet is zero. I didn't find evidence showed it would not, but I > found a lot of code might be broken if an element has no character. Hence, I > suppose that the bullet will never be empty. sometimes it is but I'd say we shouldn't have bullet accessible in this case, see bug 1015389
(In reply to alexander :surkov from comment #7) > (In reply to Xidorn Quan from comment #5) > > > > + uint32_t nextOffset = GetChildOffset(1); > > > > > > nit: good to define it where needed > > > > Since both branch in the switch use this variable, I think it is the proper > > place to define it. > > right but you still call the method for all other values not processed by > switch statement Both branch will return, and no code is executed after that switch. > > > > + case eSelectBeginLine: > > > > > > why beginLine here? > > > > Is it guaranteed that eSelectBeginLine will only be passed in with > > eDirPrevious? If so, I will remove it. > > I see what you say, iirc the current logic doesn't allow this case but I > agree it's better to have an assertion. I'll add a assertion. > > > @@ +464,3 @@ > > > > // Ask a text leaf next (if not empty) to the bullet for an offset > > > > // since list item may be multiline. > > > > + return nextOffset < CharacterCount() ? > > > > > > btw, it doesn't work when line is a list bullet only > > > > > > wouldn't it nicer to compare aOffset and nextOffset instead? > > > > It does work. This patch itself is able to pass test_lineboundary.html. > > I don't see a proper test case in the mochitest. Say you have an example > <li><br>hello</li> > line is ending at nextOffset, nextOffset is lesser than CharacterCount and > you call FindOffset for 'hello' text node what returns end of 'hello'. Does > it sound correct? It does correctly stop at the <br> which generates an "\n". I'll add a test for that. > > The > > only problem I care about is that there will be an infinite recursion if > > length of bullet is zero. I didn't find evidence showed it would not, but I > > found a lot of code might be broken if an element has no character. Hence, I > > suppose that the bullet will never be empty. > > sometimes it is but I'd say we shouldn't have bullet accessible in this > case, see bug 1015389 It isn't the only case. @counter-style may generate bullet with empty content as well. I think it should be processed in a higher position, but I'll add a check here to prevent infinite recursion.
(In reply to Xidorn Quan from comment #8) > Both branch will return, and no code is executed after that switch. I meant you don't to have an extra call for example for eStartWord value which is not handled by switch, right? > > I see what you say, iirc the current logic doesn't allow this case but I > > agree it's better to have an assertion. > > I'll add a assertion. thank you > > I don't see a proper test case in the mochitest. Say you have an example > > <li><br>hello</li> > > line is ending at nextOffset, nextOffset is lesser than CharacterCount and > > you call FindOffset for 'hello' text node what returns end of 'hello'. Does > > it sound correct? > > It does correctly stop at the <br> which generates an "\n". I'll add a test > for that. ok, I can't think of soft line break example so I'm good with it. > > sometimes it is but I'd say we shouldn't have bullet accessible in this > > case, see bug 1015389 > > It isn't the only case. @counter-style may generate bullet with empty > content as well. I think it should be processed in a higher position, but > I'll add a check here to prevent infinite recursion. thank you, will you upload new patch?
(In reply to alexander :surkov from comment #9) > (In reply to Xidorn Quan from comment #8) > > > Both branch will return, and no code is executed after that switch. > > I meant you don't to have an extra call for example for eStartWord value > which is not handled by switch, right? It will always return nextOffset directly for all cases other than eSelectLine and eSelectEndLine. eStartWord is one case in that, isn't it? > > > sometimes it is but I'd say we shouldn't have bullet accessible in this > > > case, see bug 1015389 > > > > It isn't the only case. @counter-style may generate bullet with empty > > content as well. I think it should be processed in a higher position, but > > I'll add a check here to prevent infinite recursion. > > thank you, will you upload new patch? I will do that soon.
Attached patch patch (obsolete) — — Splinter Review
Attachment #8429216 - Attachment is obsolete: true
Attachment #8429216 - Flags: review?(surkov.alexander)
Attachment #8429261 - Flags: review?(surkov.alexander)
(In reply to Xidorn Quan from comment #10) > (In reply to alexander :surkov from comment #9) > > (In reply to Xidorn Quan from comment #8) > > > > > Both branch will return, and no code is executed after that switch. > > > > I meant you don't to have an extra call for example for eStartWord value > > which is not handled by switch, right? > > It will always return nextOffset directly for all cases other than > eSelectLine and eSelectEndLine. eStartWord is one case in that, isn't it? right, both 1 and nextOffset are wrong in multichar bullets case though. Having nextOffset makes unclear the comment above about treating the bullet as one character. So if you want to keep nextOffset then I think it's ok but it'd be good to change the comment similiary to what we had before like "XXX: the logic is broken for multichar bullets in move by char/word cases".
(In reply to alexander :surkov from comment #12) > (In reply to Xidorn Quan from comment #10) > > (In reply to alexander :surkov from comment #9) > > > (In reply to Xidorn Quan from comment #8) > > > > > > > Both branch will return, and no code is executed after that switch. > > > > > > I meant you don't to have an extra call for example for eStartWord value > > > which is not handled by switch, right? > > > > It will always return nextOffset directly for all cases other than > > eSelectLine and eSelectEndLine. eStartWord is one case in that, isn't it? > > right, both 1 and nextOffset are wrong in multichar bullets case though. > Having nextOffset makes unclear the comment above about treating the bullet > as one character. So if you want to keep nextOffset then I think it's ok but > it'd be good to change the comment similiary to what we had before like > "XXX: the logic is broken for multichar bullets in move by char/word cases". I'm fine with the comment you suggest. Please check other parts of the patch. I'll update the patch tomorrow.
Comment on attachment 8429261 [details] [diff] [review] patch Review of attachment 8429261 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thank you for fixing this one! ::: accessible/src/generic/HyperTextAccessible.cpp @@ +459,2 @@ > > + uint32_t nextOffset = GetChildOffset(1); how does it work in case of bullet-only listitem? @@ +463,3 @@ > > + NS_ASSERTION(aAmount != eSelectBeginLine, > + "eSelectBeginLine should not be used with eDirNext"); it's good to have an assertion at top of the method ::: accessible/tests/mochitest/text/test_lineboundary.html @@ +126,5 @@ > [ 8, 11, "and ", 8, 12 ] ]); > testTextAtOffset([ "li4" ], BOUNDARY_LINE_START, > [ [ 0, 6, kDiscBulletChar + "a " + kEmbedChar + " c", 0, 6 ] ]); > + testTextAtOffset([ "li5" ], BOUNDARY_LINE_START, > + [ [ 0, 1, kDiscBulletChar + "\n", 0, 2 ] ]); it's good to have (2, ) line check
Attachment #8429261 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #14) > Comment on attachment 8429261 [details] [diff] [review] > patch > > Review of attachment 8429261 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, thank you for fixing this one! > > ::: accessible/src/generic/HyperTextAccessible.cpp > @@ +459,2 @@ > > > > + uint32_t nextOffset = GetChildOffset(1); > > how does it work in case of bullet-only listitem? I read the code of GetChildOffset, and found it only cares about the total length before the given index. Hence it works fine with bullet-only listitem. You can find some similiar usage of this method in the existing code.
Attached patch patch, r=surkov — — Splinter Review
Attachment #8429261 - Attachment is obsolete: true
Attachment #8429698 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fd6d8a361cee
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: