1551185 - Contenteditable Div: Inline <img> in selection are not always highlighted

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Danilo B]

Attachment: testcase.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

See attached testcase.

• Click into the contenteditable div, after the second emoji
• Press shift+left. This will add a selection around the second emoji, but it's not highlighted.
• Press shift+left again. Now both emoji are selected, but only the first is highlighted.

Note that when pressing shift+home, selection / highlighting looks correct.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This looks like a painting issue. When shifting tabs back and forth the selection is correctly repainted.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=eaf6867923b99cd189d501cd3dd131f73c0e533c&tochange=541fbb29f21d262d88d0ae08776606db0b62d8f6

Triggered by: Bug 1449564

If only one image was selected,
Before Bug 1449564, Image resizer editing UIis indicated.
Alfter Bug 1449564, Image resizer editing UI is no longer indicated as expected.
But Highlight isn't also indicated --- Bug.

So, this bug is, if editor.resizing.enabled_by_default=false, the selected img should be highlighted instead of resizer editing UI.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I think this is still a painting bug, likely it was papered before because of the editing UI.

Comment 4

[Matt Woodrow (:mattwoodrow)]

If only the second emoji is selected, then switching tabs doesn't fix the rendering issue, it's never drawn with selection.

When adding the first emoji to the selection, then the behaviour differs based on whether retained display lists is enabled.

With RDL, only the first emoji is drawn with selection, and the second remains without. Switching tabs fixes this so that both are drawn selected. This is somewhat expected, as selecting the first emoji doesn't change the selection state on the second, so RDL is correctly not redrawing it.

Without RDL, then adding the first emoji to the selection causes both to be redrawn with selection. Again, this is not unexpected, since we recreate all display items.

Comment 5

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

Must be a long standing bug of nsImageFrame::ShouldDisplaySelection(). It does not check whether editor puts resizers to the image. So, this should've been reproducible with document.execCommand("enableObjectResizing", false, false) even before Firefox 63.

I think that HTMLEditor should have a getter method returning HTMLEditor::mResizedObject and nsImageFrame::ShouldDisplaySelection() should test if it's the nsIFrame::mContent. HTMLEditor can be retrieved with nsContentUtils::GetHTMLEditor().

Comment 6

[Matt Woodrow (:mattwoodrow)]

Are you going to work on fixing this Masayuki?

Comment 7

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

Well, I have a patch for this now. Taking.

Comment 8

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

Oh, test_image_selection.html detected this bug, but I didn't realize the fact...

Comment 9

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

Attachment: Bug 1551185 - Make nsImageFrame::ShouldDisplaySelection() check whether resizer target element is its content or not

Currently, nsISelectionDisplay::DISPLAY_ALL is used only by HTMLEditor.
And only when it's set, nsImageFrame::ShouldDisplaySelection() returns false
if only its mContent is selected. However, this is based on an assumption,
that is, when only one <img> is selected in an HTML editor, it's target of
resizers. However, this is completely wrong. Web apps can disable resizers
with document.execCommand("enableObjectResizing", false, false) and now,
it's disabled by default.

Therefore, this patch makes the method check whether its mContent is
target of resizers at the moment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b67f75de3e3
Make nsImageFrame::ShouldDisplaySelection() check whether resizer target element is its content or not r=smaug

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6b67f75de3e3

Comment 12

[Julien Cristau [:jcristau]]

Is this worth fixing in beta?

Comment 13

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

(In reply to Julien Cristau [:jcristau] from comment #12)

Is this worth fixing in beta?

yeah, I think so. I'll request uplift soon.

Comment 14

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

Comment on attachment 9067288 [details]
Bug 1551185 - Make nsImageFrame::ShouldDisplaySelection() check whether resizer target element is its content or not

Beta/Release Uplift Approval Request

• User impact if declined: User may be confused by visually not selected <img> element even though it's actually selected.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1449564
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): nsImageFrame::ShouldDisplaySelection() assumed the situation, that is, only one <img> element selected and HTMLEditor has focus, the element is target of resizers. However, this is completely wrong even before the fix of bug 1449564 since web apps can disable resizers by themselves. This patch just makes it directly ask HTMLEditor whether the <img> is current target of resizers.
• String changes made/needed: none

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9067288 [details]
Bug 1551185 - Make nsImageFrame::ShouldDisplaySelection() check whether resizer target element is its content or not

fix interaction between image selection and editor, approved for 68.0b6

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/09b6c96b16ca

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/136538c5fece
followup: Give nsImageFrame.cpp an #include for HTMLEditor (a type it uses in this bug's main patch). (no review, just a trivial #include addition for correctness)

Comment 18

[Daniel Holbert [:dholbert]]

I landed a trivial followup (comment 17) to add a include that was missing in the main patch here, to provide the HTMLEditor type definition for the new call to htmlEditor->GetResizerTarget() here:
https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/layout/generic/nsImageFrame.cpp#2142-2147

We probably don't need to bother uplifting that to beta (though we could probably do so without any risk, since it's a non-functional change, and there's a chance it'd benefit some folks who tend to hit unified-build-related bustage like folks who maintain some of our cross-platform ports).

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks, Daniel, the TB people hit unified-build-related bustage from time to time, would be good to avoid that.

Comment 20

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

Thank you for the follow up fix, dholbert!

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/136538c5fece