1368521 - Selection invalidation is broken in nightly 2017-05-29

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

STR:
Open http://searchfox.org/mozilla-central/source/testing/xpcshell/selftest.py#1010

Try to select some text by dragging up and down over multiple lines.

Result:
Some text will appear not selected or selected despite being the opposite.

Expected Result:
Text will be selected normally.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

[Tracking Requested - why for this release]:

User-visible regression for text selection.

Comment 2

[Timothy Nikkel (:tnikkel)]

mozregression says it's caused by bug 1367690.

Comment 3

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

Odd, I tried to test it on simple pages, but I didn't see the regression.

Comment 4

[Julien Cristau [:jcristau]]

tracking as new regression in 55.

Comment 5

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

Attachment: Bug 1368521 Selection::SelectFrames() shouldn't skip first content invalidation if it's different from handled text node

The first node of |iter| in Selection::SelectFrames() may be different from the start parent of given range.  Therefore, if they are different, it shouldn't skip first item of |iter|.

Review commit: https://reviewboard.mozilla.org/r/144096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144096/

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8872569 [details]
Bug 1368521 Selection::SelectFrames() shouldn't skip first content invalidation if it's different from handled text node

https://reviewboard.mozilla.org/r/144096/#review147800

Please add a regression test that fails without this patch.

Comment 7

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

(In reply to Mats Palmgren (:mats) from comment #6)
> Comment on attachment 8872569 [details]
> Bug 1368521 Selection::SelectFrames() shouldn't skip first content
> invalidation if it's different from handled text node
> 
> https://reviewboard.mozilla.org/r/144096/#review147800
> 
> Please add a regression test that fails without this patch.

Hmm, I can reproduce the bug with synthesizeKey(). However, it cannot be captured as expected with snapshotWindow() of WindowSnapshot.js. I guess that it draws "current" paint result rather than copies the bitmap of actual painted result. I.e., it might not be the result of painted a lot with invalidated a lot.

So, I don't have idea to capture the buggy result of this regression. Do you have any ideas?

Comment 8

[Mats Palmgren (inactive)]

I don't really understand why snapshotWindow() wouldn't work either.
Perhaps the test could wait for MozReftestInvalidate and then compare
window.mozPaintCount before and after doing the selection?  That
wouldn't capture that we painted it correctly but at least that
something was painted.  If that doesn't work you'd have to ask
someone more familiar with painting.

Comment 9

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

(In reply to Mats Palmgren (:mats) from comment #8)
> I don't really understand why snapshotWindow() wouldn't work either.

The snapshot includes selection, not not-includes selection due to capturing before paint. I always got ideal result even if the painting on screen is broken.

It orders a canvas element to paint a window:
https://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/testing/specialpowers/content/specialpowersAPI.js#1442-1443,1464-1468

So, I believe that it never can catch invalidation regression.

> Perhaps the test could wait for MozReftestInvalidate and then compare
> window.mozPaintCount before and after doing the selection?

MozReftestInvalidate is fired by reftest framework:
https://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/layout/tools/reftest/reftest-content.js#535-536
And we cannot use reftest since reftest cannot synthesize user input events.

> That wouldn't capture that we painted it correctly but at least that
> something was painted.  If that doesn't work you'd have to ask
> someone more familiar with painting.

mattwoodrow:

I'd like to get a snapshot of failing to frame invalidation during selection change. Is it possible to get/test with mochitest? If it's possible how to get/test it?

Comment 10

[Matt Woodrow (:mattwoodrow)]

The MozAfterPaint includes the rectangles of what got invalidated.

The reftest harness uses these rectangles to restrict the DrawWindow calls to just the area that reported as changed, and uses that to test invalidation.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Also, can you trigger the bug by changing selection from JS? Or is it only from user input?

If it's the former then you can use a MozReftestInvalidate/reftest-wait reftest to test it.

We could also probably add support for synthesizing events within reftests if needed.

Comment 13

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> The MozAfterPaint includes the rectangles of what got invalidated.
> 
> The reftest harness uses these rectangles to restrict the DrawWindow calls
> to just the area that reported as changed, and uses that to test
> invalidation.

Thank you for the information.

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Also, can you trigger the bug by changing selection from JS? Or is it only
> from user input?

Good point. Getting different result, I got the buggy result with emulating same operation with Selection API.

> If it's the former then you can use a MozReftestInvalidate/reftest-wait
> reftest to test it.

Thanks. I'll try to do rewrite my test with reftests.

> We could also probably add support for synthesizing events within reftests
> if needed.

If it's possible (and perhaps, if SpecialPowers are available in reftests), some mochitests are rewritten with simpler reftests, though.

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e85da080583ef7fa858949ff5f3cb472afa8b1a7

Comment 15

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

Comment on attachment 8872569 [details]
Bug 1368521 Selection::SelectFrames() shouldn't skip first content invalidation if it's different from handled text node

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144096/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5f7de87eceb
Selection::SelectFrames() shouldn't skip first content invalidation if it's different from handled text node r=mats

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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