Closed Bug 1374999 Opened 2 years ago Closed 2 years ago

stylo: Image resizer in contenteditable doesn't show up

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

For the following code:
<div contenteditable>
  <img src="x" width="100" height="100">
</div>

When clicking the img element, eight resizers should be shown around the element.

But it doesn't work in Stylo.

The resizers are created in HTMLEditorObjectResizer.cpp [1]. I guess either we fail to restyle the anonymous elements, or we fail to handle their rules [2] correctly.

This is the reason of failures inside editor/libeditor/tests/test_bug640321.html.


[1] https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/editor/libeditor/HTMLEditorObjectResizer.cpp#292
[2] https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/layout/style/contenteditable.css#109
heycam, any thought?
Flags: needinfo?(cam)
Off the top of my head, I don't know.  It's definitely different from the <textarea> resizer problems (which should now be fixed with the initial XBL support landed).  It might be something to do with the strange way these elements are created with http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/editor/libeditor/HTMLAnonymousNodeEditor.cpp#165.

I'll look into it.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
I'm guessing the following tests are affected by the same issue:
* editor/libeditor/tests/test_inlineTableEditing.html
* editor/libeditor/tests/test_objectResizing.html
(In reply to Cameron McCormack (:heycam) from comment #2)
> t might be something to do with the strange way these
> elements are created with
> http://searchfox.org/mozilla-central/rev/
> 714606a8145636d93b116943d3a65a6a49d2acf8/editor/libeditor/
> HTMLAnonymousNodeEditor.cpp#165.

I think it is related to this.  That HTMLEditor::CreateAnonymousElement function creates a <span> for the resizer, marks it as a NAC root, and binds it to the <div contenteditable>.  It then calls nsIPresShell::PostRecreateFramesFor.  In Gecko, this has the effect of indeed creating a frame for the element.  Subsequent restyles will then find the element, as GeckoRestyleManager traverses the frame tree.  In Stylo, the PostRecreateFramesFor does nothing, since the element has no ServoElementData, and we just drop the request to post an explicit ReconstructFrame change hint.  Even if that succeeded, though, Servo's DOM-based traversal won't find the resizer <span>, because we only find NAC (in AllChildrenIterator) that was created by an nsIAnonymousContentCreator-implementing frame.

In place of PostRecreateFramesFor, maybe we can just directly call nsCSSFrameConstructor::ContentRangeInserted.  For finding this content, I suppose we should hang it off the parent element somehow, perhaps in a property, with a flag we can check quickly to avoid looking up the property table when we can.
Attached patch WIPSplinter Review
This patch basically works, though will be slow due to the DOM property lookups.
Hmm, no free node bits currently.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> I'm guessing the following tests are affected by the same issue:
> * editor/libeditor/tests/test_inlineTableEditing.html
> * editor/libeditor/tests/test_objectResizing.html

These tests pass with the patch.
Comment on attachment 8881107 [details]
Bug 1374999 - stylo: Iterate over manually created editor NAC.

https://reviewboard.mozilla.org/r/152414/#review157494

Seems like this leaks. Who deletes the nsTArray?

Seems like it's also worth making this an nsAutoTArray or something, so that we don't have to heap-allocate twice.


(In reply to Cameron McCormack (:heycam) from comment #6)
> Hmm, no free node bits currently.

There are two free bool flags after https://hg.mozilla.org/mozilla-central/rev/3947ddaad2c3 . Though I think we should use the same bit as bug 1372061.

I would generally prefer this bug to land after that one, since otherwise the traversal will get slower. Is that ok? What's your timeline on bug 1372061?
Attachment #8881107 - Flags: review?(bobbyholley) → review-
Comment on attachment 8881107 [details]
Bug 1374999 - stylo: Iterate over manually created editor NAC.

https://reviewboard.mozilla.org/r/152414/#review157494

The DeleteProperty call invokes the property deleter that was passed in in the SetProperty call.

nsAutoTArray sounds like a good idea.

Thanks for the pointer to the bool flags.  I only checked the node flags!

I haven't started yet on bug 1372061.  That's next.  I'll get that done first and rebase on top of it.
(In reply to Cameron McCormack (:heycam) from comment #11)
> Comment on attachment 8881107 [details]
> Bug 1374999 - stylo: Iterate over manually created editor NAC.
> 
> https://reviewboard.mozilla.org/r/152414/#review157494
> 
> The DeleteProperty call invokes the property deleter that was passed in in
> the SetProperty call.

Ok ok - and that's guaranteed to be invoked by the time a node is destroyed?
Yes, if the property gets left on the node, the destruction function will be called at that time.  And it gets called if the property is explicitly removed with DestroyProperty.
Depends on: 1372061
Comment on attachment 8881107 [details]
Bug 1374999 - stylo: Iterate over manually created editor NAC.

https://reviewboard.mozilla.org/r/152414/#review157922
Attachment #8881107 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bca2476e0117
stylo: Iterate over manually created editor NAC. r=bholley
https://hg.mozilla.org/mozilla-central/rev/bca2476e0117
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1377256
This is not fixed. The resizers are still not displayed.
Status: RESOLVED → REOPENED
Flags: needinfo?(cam)
Resolution: FIXED → ---
Or it is also possible that this was regressed in the past month because the test is not enabled.
Yeah, looks like it is a regression.  I'll follow up in a new bug.
Flags: needinfo?(cam)
Blocks: 1383988
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Depends on: 1386110
You need to log in before you can comment on or make changes to this bug.