Closed Bug 1374999 Opened 2 years ago Closed 2 years ago
stylo: Image resizer in contenteditable doesn't show up
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 . I guess either we fail to restyle the anonymous elements, or we fail to handle their rules  correctly. This is the reason of failures inside editor/libeditor/tests/test_bug640321.html.  https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/editor/libeditor/HTMLEditorObjectResizer.cpp#292  https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/layout/style/contenteditable.css#109
heycam, any thought?
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
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.
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.
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/bca2476e0117 stylo: Iterate over manually created editor NAC. r=bholley
This is not fixed. The resizers are still not displayed.
Status: RESOLVED → REOPENED
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.
Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.