1413497 - stylo: tests/editor/reftests/824080-2.html test bug fails on Android/styloVsGecko

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/logviewer.html#?job_id=140637195&repo=try&lineNumber=7926

AcceibilityCaret state is different on styloVsGecko.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Makoto san, did you start looking this?  I think you know about editor code better than me, but if you have no time, I will take a look.

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Makoto san, did you start looking this?  I think you know about editor code
> better than me, but if you have no time, I will take a look.

OK, but this isn't editor issue and is selection.

Comment 3

[Makoto Kato [:m_kato]]

Maybe, this is test bug.  When I modify reftest.list to "need-focus == tests/editor/reftests/824080-2.html tests/editor/reftests/824080-2.html", this test is always failed without stylo.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thanks!

I've looked this a bit.  I don't know what the AccesibleCaret is actually, but if it's the blue shape like circle, I'd say this is also Gecko's failure, the AccesibleCaret should not be appeared if layout.accessiblecaret.enabled is false, right?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Also note that these tests (824080-x.html) seem to be fundamentally broken, all of them should use reftest-wait there.
CCing Masayuki.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Thanks!
> 
> I've looked this a bit.  I don't know what the AccesibleCaret is actually,
> but if it's the blue shape like circle, I'd say this is also Gecko's
> failure, the AccesibleCaret should not be appeared if
> layout.accessiblecaret.enabled is false, right?

No, on Android, layout.accessiblecaret.enabled_on_touch is true, so AccesibleCaret should be shown.  But bug 1195722 forgets to add pref(layout.accessiblecaret.enabled,false) pref(layout.accessiblecaret.enabled_on_touch,false) for this test.

I think that root cause is android's reftest harness.  When investigating this, blur event occurs on reference test.  Example, when adding need-focus == tests/editor/reftests/824080-2.html tests/editor/reftests/824080-2.html to reftest.list, this test is always failure even if same html.


(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Also note that these tests (824080-x.html) seem to be fundamentally broken,
> all of them should use reftest-wait there.
> CCing Masayuki.

Even if adding reftest-wait, this issue occurs.  Because reftest harness for reference test seems to occur blur.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not familiar with AccessibleCaret, though, it uses script runner for changing caret state:
https://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/layout/base/AccessibleCaretManager.cpp#1487

I'm also not familiar with script runner at loading, though, does it change the result if doTest() is run with setTimeout(doTest, 0)? Then, the function should always run at safe time. So, the event is always dispatched synchronously.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Makoto Kato [:m_kato] from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > Thanks!
> > 
> > I've looked this a bit.  I don't know what the AccesibleCaret is actually,
> > but if it's the blue shape like circle, I'd say this is also Gecko's
> > failure, the AccesibleCaret should not be appeared if
> > layout.accessiblecaret.enabled is false, right?
> 
> No, on Android, layout.accessiblecaret.enabled_on_touch is true, so
> AccesibleCaret should be shown.  But bug 1195722 forgets to add
> pref(layout.accessiblecaret.enabled,false)
> pref(layout.accessiblecaret.enabled_on_touch,false) for this test.

Oh, I thought the failure entry is

 fuzzy-if(OSX,1,1) needs-focus pref(layout.accessiblecaret.enabled,false) pref(layout.accessiblecaret.enabled_on_touch,false) == 824080-2.html 824080-2-ref.html #Bug 1313253

but actually

 needs-focus != 824080-2.html 824080-3.html

?
 
> I think that root cause is android's reftest harness.  When investigating
> this, blur event occurs on reference test.  Example, when adding need-focus
> == tests/editor/reftests/824080-2.html tests/editor/reftests/824080-2.html
> to reftest.list, this test is always failure even if same html.

So, I am still wondering why we don't need to set those two pref values false there?
 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > Also note that these tests (824080-x.html) seem to be fundamentally broken,
> > all of them should use reftest-wait there.
> > CCing Masayuki.
> 
> Even if adding reftest-wait, this issue occurs.  Because reftest harness for
> reference test seems to occur blur.

Yep, I don't say reftest-wait fixes this failure, I am just saying these tests originally has not been worked as expected.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> I'm not familiar with AccessibleCaret, though, it uses script runner for
> changing caret state:
> https://searchfox.org/mozilla-central/rev/
> 423b2522c48e1d654e30ffc337164d677f934ec3/layout/base/AccessibleCaretManager.
> cpp#1487
> 
> I'm also not familiar with script runner at loading, though, does it change
> the result if doTest() is run with setTimeout(doTest, 0)? Then, the function
> should always run at safe time. So, the event is always dispatched
> synchronously.

Yeah, it seems to work, if we can observe mozcaretstatechanged in reftest, we should wait for the event instead.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Confirmed that setting the pref makes the styloVsGecko pass (Honestly it's trivial since we have tested it on the above entry).

(In reply to Makoto Kato [:m_kato] from comment #3)
> Maybe, this is test bug.  When I modify reftest.list to "need-focus ==
> tests/editor/reftests/824080-2.html tests/editor/reftests/824080-2.html",
> this test is always failed without stylo.

Anyway we should open a new bug for it.

Comment 11

[Chris Peterson [:cpeterson]]

I'm reducing this bug's priority from P2 to P3 because this is a test bug and does not need to block shipping Stylo-android.