1374999 - stylo: Image resizer in contenteditable doesn't show up

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

heycam, any thought?

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I'm guessing the following tests are affected by the same issue:
* editor/libeditor/tests/test_inlineTableEditing.html
* editor/libeditor/tests/test_objectResizing.html

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: WIP

This patch basically works, though will be slow due to the DOM property lookups.

Comment 6

[Cameron McCormack (:heycam)]

Hmm, no free node bits currently.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1374999 - stylo: Iterate over manually created editor NAC.

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

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Cameron McCormack (:heycam)]

(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 10

[Bobby Holley (:bholley)]

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?

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Bobby Holley (:bholley)]

(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?

Comment 13

[Cameron McCormack (:heycam)]

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.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8881107 [details]
Bug 1374999 - stylo: Iterate over manually created editor NAC.

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

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8881107 [details]
Bug 1374999 - stylo: Iterate over manually created editor NAC.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bca2476e0117
stylo: Iterate over manually created editor NAC. r=bholley

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

This is not fixed. The resizers are still not displayed.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Or it is also possible that this was regressed in the past month because the test is not enabled.

Comment 20

[Cameron McCormack (:heycam)]

Yeah, looks like it is a regression.  I'll follow up in a new bug.