Bug 161546 – Need to updateEditorState if an element change edit-ability without changing selection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Beth Dakin]

Need to updateEditorState if an element change edit-ability without changing selection

rdar://problem/27806012

[Beth Dakin]

Created attachment 287811 [details]
Patch

[Build Bot]

Comment on attachment 287811 [details]
Patch

Attachment 287811 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1996343

New failing tests:
editing/secure-input/removed-password-input.html

[Build Bot]

Created attachment 287817 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

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.

> Source/WebCore/editing/FrameSelection.cpp:2386
> +        client->updateEditorStateAfterLayoutIfNeeded();

Instead of saying IfNeeded, can we explicitly say IfEditabilityChanged to make semantics clear?

> Source/WebCore/page/EditorClient.h:187
> +
> +    virtual void updateEditorStateAfterLayoutIfNeeded() = 0;

We should add this right at where didChangeSelectionAndUpdateLayout is declared.

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:557
> +void WebEditorClient::updateEditorStateAfterLayoutIfNeeded()
> +{
> +    m_page->updateEditorStateAfterLayoutIfNeeded();
> +}
> +

Ditto about defining this next to didChangeSelectionAndUpdateLayout.

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

Ditto about the location.

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

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1479
> +    EditorStateIsContentEditable m_lastEditorStateWasContentEditable { EditorStateIsContentEditable::Unset };

Should we just make this mutable instead of removing const from editorState?

[Beth Dakin]

(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

[Ryosuke Niwa]

Windows build fix landed in http://trac.webkit.org/changeset/205393.