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

NEW
Assigned to

Status

()

defect
P3
normal
2 years ago
a year ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 affected)

Details

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.
Flags: needinfo?(m_kato)
Assignee

Comment 2

2 years ago
(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.
Flags: needinfo?(m_kato)
Assignee

Comment 3

2 years ago
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.
Assignee: nobody → m_kato
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?
Also note that these tests (824080-x.html) seem to be fundamentally broken, all of them should use reftest-wait there.
CCing Masayuki.
Assignee

Comment 6

2 years ago
(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.
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.
(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.
(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.
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.
Priority: -- → P2
Assignee

Updated

2 years ago
See Also: → 1415008
Assignee

Updated

2 years ago
Blocks: 1406290
No longer blocks: stylo-android
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.
Priority: P2 → P3
Summary: stylo: tests/editor/reftests/824080-2.html is failure on Android/styloVsGecko → stylo: tests/editor/reftests/824080-2.html test bug fails on Android/styloVsGecko
You need to log in before you can comment on or make changes to this bug.