Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 10 years ago

Line boundary is incorrect for bullets have more than one character.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 10 years ago

tryserver: https://tbpl.mozilla.org/?tree=Try&rev=3d1f8102f6ca

	alexander :surkov (:asurkov)
	Comment 4 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 10 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 7 • 10 years ago

(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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 8 • 10 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 9 • 10 years ago

(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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 10 • 10 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 12 • 10 years ago

(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".

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 13 • 10 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 14 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 15 • 10 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 17 • 10 years ago

tryserver: https://tbpl.mozilla.org/?tree=Try&rev=8242da1ac425

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6d8a361cee

	Carsten Book [:Tomcat]
	Comment 19 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fd6d8a361cee