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

RESOLVED FIXED in Firefox 68

Status

()

defect
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: gezuru, Assigned: masayuki)

Tracking

(Regression, {regression})

68 Branch
mozilla69
Desktop
All
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 wontfix, firefox68 fixed, firefox69 fixed)

Details

Attachments

(2 attachments)

Posted file 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.

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

Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Editor → Web Painting
Ever confirmed: true

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.

OS: Unspecified → All
Hardware: Unspecified → Desktop
Flags: needinfo?(masayuki)

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

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.

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().

Flags: needinfo?(masayuki)

Are you going to work on fixing this Masayuki?

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

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Priority: -- → P2

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.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6b67f75de3e3
Make nsImageFrame::ShouldDisplaySelection() check whether resizer target element is its content or not r=smaug
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Is this worth fixing in beta?

Flags: needinfo?(masayuki)

(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 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
Flags: needinfo?(masayuki)
Attachment #9067288 - Flags: approval-mozilla-beta?

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

Attachment #9067288 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by dholbert@mozilla.com:
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)

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).

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

You need to log in before you can comment on or make changes to this bug.