Open Bug 1667599 Opened 4 years ago Updated 4 years ago

"highlight all" highlights wildly unrelated strings

Categories

(Core :: DOM: Selection, defect, P3)

Firefox 83
defect

Tracking

()

People

(Reporter: pomax, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Attached image screenshot.361.png (deleted) —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

The attachment has all the information: "highlight all" is doing rather the wrong thing.

Can you attach or link to the page that causes this? Without that investigating is going to be hard.

Flags: needinfo?(pomax)
Flags: needinfo?(pomax)

Specifically for that screenshot: https://pomax.github.io/bezierinfo/#molding

Thanks! I could repro that. It worked in 68 so it's a regression. I got this regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4a19c3f74d0cf11a6a2dc143b3b43a88a96f0932&tochange=518df4329a20f76b3bffd5f6201b007fdebc274f

Which is a bit odd because I don't see anything particularly find-specific, so probably we should try to narrow it down a bit. Maybe some of the graphics changes in there?

(Btw, nice page! I remember reading it a while ago, and it was really interesting :))

Hah, yes that's quite the range... hopefully hg has a bisect command?

Also, thanks, always good to hear from folks who found it interesting!

(In reply to Pomax from comment #5)

Hah, yes that's quite the range... hopefully hg has a bisect command?

Yeah, mozregression usually gets closer than that, but for older builds it sometimes chokes because they've been cleaned up or what not.

Bisecting using hg bisect or git bisect is a bit more painful because it means you have to build the revisions, and building a year-old tree happens to be more painful than it seems (due to different toolchain and other dependency versions...). Anyhow, I'll try to manually bisect that since this bug is a bit concerning. Though a bit swamped with other stuff so may take a bit.

Thanks a lot for reporting it!

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(emilio)
Keywords: regression

Huh, I was not quite expecting this... I bisected this down to bug 1566141 o.O

At first I thought I had to be wrong, but I can reliably reproduce it. If I check out https://hg.mozilla.org/mozilla-central/rev/28aa763e7834023b28c2462a078f1bd91baa7f7b it's bad. If I revert that patch then it's good.

So maybe some weird behavior change in our find JS code? Something about our bytecode serialization or something that may have broken? Yulia, do you have any idea of what may be going on here?

I'll look into it a bit, in the "bad" case we get a bunch of non-fatal assertions here, so I should be able to track them down and see what's going on... But how nullish coalescing can end up in our find code being broken is just amazing.

Flags: needinfo?(ystartsev)
Regressed by: 1566141

Ah, silly me, of course there's an easier explanation. The website uses ??, and thus it throws in older versions of the browser. That means it doesn't create the shadow roots from <graphics-element>s which are what cause the weirdness...

So I guess it's not technically a regression. Still we should probably handle this somehow.

Depends on: 1590379
Flags: needinfo?(ystartsev)
Keywords: regression
No longer regressed by: 1566141

So, this is a test-case that can show some of the weirdness. It can probably be tweaked to be even weirder. Most selections just not show up and when you're past the middle of the matches you can see that the whole word is incorrectly highlighted.

Highlight all doesn't deal well with Shadow DOM, because IsItemInRangeComparator doesn't deal well with disconnected ranges (so the binary search doesn't end up quite working).

But I'm a bit confused about why do we need to go from ranges to selection, then binary-search all ranges in a selection? It seems the ranges we gather for GetExistingClosestCommonInclusiveAncestorRanges should be a pretty small set usually (at least for the find-in-page code). When is it not?

Flags: needinfo?(mbrodesser)
Flags: needinfo?(bugs)

Hmm, looking a bit a the history of this code... bug 1374338 and bug 1216001 are the two sources of complexity here. So table selection + bits of user-select: none seems to be the two things that can cause the list of ranges to grow quite high for some nodes...

The order is broken, too. In the test-case we're hitting this fairly frequently...

Mirko, this seems like the kind of stuff you are looking into as part of bug 1590379, but let me know if I can help here somehow.

Component: Find Backend → DOM: Selection
Flags: needinfo?(emilio)
Flags: needinfo?(bugs)

But I'm a bit confused about why do we need to go from ranges to selection, then binary-search all ranges in a selection? It seems the ranges we gather for GetExistingClosestCommonInclusiveAncestorRanges should be a pretty small set usually (at least for the find-in-page code). When is it not?

AFAIK, spellchecking can create many ranges too. Each misspelling instance corresponding to one range.

The order is broken, too. In the test-case we're hitting this fairly frequently...

Mirko, this seems like the kind of stuff you are looking into as part of bug 1590379, but let me know if I can help here somehow.

AFAICT, the underlying problem seems related to Shadow DOM and the ordering of the ranges, yes. So "DOM:Selection" is presumably the correct component. I'm currently not working on bug 1590379 but plan to in the future.

Flags: needinfo?(mbrodesser)
Severity: -- → S3
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: