[Webkit-unassigned] [Bug 161546] Need to updateEditorState if an element change edit-ability without changing selection

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=161546

Beth Dakin <bdakin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Beth Dakin <bdakin at apple.com> ---
(In reply to comment #4)
> Comment on attachment 287811 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287811&action=review
> 
> r=me assuming you fix builds & tests and stuff.

Thanks!

> 
> > Source/WebCore/editing/FrameSelection.cpp:2386
> > +        client->updateEditorStateAfterLayoutIfNeeded();
> 
> Instead of saying IfNeeded, can we explicitly say IfEditabilityChanged to
> make semantics clear?
> 

Yes, fixed.

> > Source/WebCore/page/EditorClient.h:187
> > +
> > +    virtual void updateEditorStateAfterLayoutIfNeeded() = 0;
> 
> We should add this right at where didChangeSelectionAndUpdateLayout is
> declared.
> 

Moved.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:557
> > +void WebEditorClient::updateEditorStateAfterLayoutIfNeeded()
> > +{
> > +    m_page->updateEditorStateAfterLayoutIfNeeded();
> > +}
> > +
> 
> Ditto about defining this next to didChangeSelectionAndUpdateLayout.
> 

Moved.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h:172
> > +    void updateEditorStateAfterLayoutIfNeeded() final;
> > +
> 
> Ditto about the location.
> 

Moved.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:910
> > +    EditorStateIsContentEditable editorStateIsContentEditable = frame.selection().selection().isContentEditable() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No;
> 
> This doesn't detect the case when the value changes between plaintextonly
> and true.
> It's probably not a big deal but you can check that condition by checking
> both isContentEditable and isContentRichlyEditable.
> You might wanna add FIXME for now.
> 

OH, good point. FIXME for now.

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:1479
> > +    EditorStateIsContentEditable m_lastEditorStateWasContentEditable { EditorStateIsContentEditable::Unset };
> 
> Should we just make this mutable instead of removing const from editorState?

Yeah, sure!

https://trac.webkit.org/changeset/205381

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160902/0fba71ca/attachment.html>