Bug 39854 – Refactor platform dependent editing behavior code out of Settings

• Component: HTML Editing
• Status: RESOLVED FIXED

[Antonio Gomes]

... as discussed suggested by Darin and Ojan in bug 36627

"Once we have a third behavior or fourth, I think we need some functions that translate the overall behavior policy into specific behaviors rather than directly saying "behavior == Mac" or "behavior == Windows". Helper functions that expression the different rules in plain language.

I think that should be straightforward and we can do that refactoring before adding a new behavior value."


+++ This bug was initially created as a clone of Bug #36627 +++

[Antonio Gomes]

Created attachment 57277 [details]
wip - first materialization of the idea

A quick prototype of the idea we discussed.

Basically, it adds a EditorSettings class, also owned by Page. The idea is to gradually move all EditingBehavior stuff in Settings to this new EditorSettings class, and change callsites of Settings::editorBehavior() to use the methods we add into this class.

I've showed how it is going to happen in the RenderBlock case, for now. (please ignore the method name)

Darin, Ojan, could you please validate that before I go on?

[Ojan Vafai]

Comment on attachment 57277 [details]
wip - first materialization of the idea

This is close to what I had in mind. Except I was picturing EditingBehavior that is a member of Settings. There's a lot of plumbing to get at Settings (e.g. Frame and Document already have settings methods). Actually, I was originally picturing something more like WebCore/editing/htmlediting.h, but the fact that the behavior is runtime settable means we can't just use a bunch of helper functions like that. I think we probably don't need the cpp file though. These methods should all be really dumb if-statements that check the editing behavior and return booleans.

That's all nit-picking though. What you have seems fine to me. I defer to darin here though.

[Darin Adler]

I originally had imagined an enum and functions that took that enum as an argument.

A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.

[Ojan Vafai]

(In reply to comment #3)
> A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.

I don't feel strongly, but this sounds best to me.

[Antonio Gomes]

Created attachment 57331 [details]
wip - second prototype of the idea

> I think we probably don't need the cpp file though. These methods should all > be really dumb if-statements that check the editing behavior and return
> booleans.

Not using dedicated .cpp/.h files for the class anymore. Instead it is not a private class of Settings and it is now called EditingBehaviorController

> A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.

Ok, also made it not mutable: wheneven Settings::setEditingBehavior(EditingBehavior) is called, a new Object is created on the stack.

In this design, for each specific editing behavior there would be a method in Settings and a correspondent method in EditingBehaviorController. The former returns a forward the call of the latter.

The changes in RenderBlock is still shown as an example call site.

What do you think?

[Darin Adler]

(In reply to comment #5)
> In this design, for each specific editing behavior there would be a method in Settings and a correspondent method in EditingBehaviorController. The former returns a forward the call of the latter.

General idea looks good, but I think this is something to be avoided. We don't want to have to add two functions every time we add a new editing behavior.

I suggest making the "controller" class public, and in fact the class itself could be in a separate header file and not be a member of settings. There should be a getter on the Settings class that returns it with a short name. Then you can say:

    settings->editing().shouldMoveCaretToBoundary()

The only part I'm not sure of is what the optimal name for EditingBehaviorController and Settings::editing() are. I think the object represents editing behavior details, interpretation, or policy. Controller might be an OK term too. For the Settings member, the key is brevity.

[Darin Adler]

Comment on attachment 57331 [details]
wip - second prototype of the idea

> +    bool moveCaretToBoundary = settings && settings->shouldMoveCaretToBoundary();

This will need to have a longer name like:

    shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom

Maybe not that exact name, but one more like it.

> -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> -        ASSERT(!useWindowsBehavior);
> +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> +        ASSERT(moveCaretToBoundary);

I think it's OK, and clearer, for the comment to call this "Mac behavior".

It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.

[Ojan Vafai]

(In reply to comment #7)
> > -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> > -        ASSERT(!useWindowsBehavior);
> > +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> > +        ASSERT(moveCaretToBoundary);
> 
> I think it's OK, and clearer, for the comment to call this "Mac behavior".

In this specific case, once we add Linux behavior in, it will apply to Mac and Linux. But in that case we could just change "Mac" to "Mac/Linux".

> It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.

This sounds great! The null-checks are super-annoying.

[Darin Adler]

(In reply to comment #8)
> (In reply to comment #7)
> > > -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> > > -        ASSERT(!useWindowsBehavior);
> > > +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> > > +        ASSERT(moveCaretToBoundary);
> > 
> > I think it's OK, and clearer, for the comment to call this "Mac behavior".
> 
> In this specific case, once we add Linux behavior in, it will apply to Mac and Linux. But in that case we could just change "Mac" to "Mac/Linux".

Right, but I think even if the comment says "the Mac behavior" it's OK if it's actually used on more platforms than just Mac.

> > It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.
> 
> This sounds great! The null-checks are super-annoying.

Maybe Editor is the right object to put this on, rather than Document, though.

[Antonio Gomes]

maybe EditingBehaviorController naming the class and EditingBehaviorPolicy naming the enum?

[Antonio Gomes]

Eric suggested something else, which sounds reasonable to me: EditingBehavior (the class) to be very self contained and not even know about Settings and all.

<eseidel> anyway if you have EditingBehavior(settings->editingBehavior()) then you can ask nice questions like .shouldIDoAwesomeEditingStuff()
<eseidel> EditingBehavior is simply about trying to make the individual code lines not have to make policy decisions about mac vs. linux
<eseidel> they ask the question they want, in "plain english" as functions on the EditingBehavior, adn that class maps from the policy enum to a bool for that behavior
<eseidel> tonikitoo: so I don't think that SEttings would even know about the new EditingBehavior
<eseidel> tonikitoo: it could, but if it does, only to just call EditingBehavior(editingBehavior())

... we would end up with call sites doing

EditingBehavior foo(settings->editingBehavior());
if (foo.shouldXXXYYYZZZ()) {
   // cool stuff
}

of course, enum would be renamed to something else to avoid collision.

thoughts?

ps: it is good that we are putting cards over table, and I am sure we will find out the solution that all would be happy in the end

[Darin Adler]

> <eseidel> anyway if you have EditingBehavior(settings->editingBehavior()) then you can ask nice questions like .shouldIDoAwesomeEditingStuff()
> <eseidel> EditingBehavior is simply about trying to make the individual code lines not have to make policy decisions about mac vs. linux
> <eseidel> they ask the question they want, in "plain english" as functions on the EditingBehavior, adn that class maps from the policy enum to a bool for that behavior

Sure that's the right concept. I agree with Eric.

> <eseidel> tonikitoo: so I don't think that SEttings would even know about the new EditingBehavior
> <eseidel> tonikitoo: it could, but if it does, only to just call EditingBehavior(editingBehavior())
> 
> ... we would end up with call sites doing
> 
> EditingBehavior foo(settings->editingBehavior());
> if (foo.shouldXXXYYYZZZ()) {
>    // cool stuff
> }
> 
> of course, enum would be renamed to something else to avoid collision.
> 
> thoughts?

I think if taken literally it will make the code at the call site too wordy. I want something more like this:

    if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())

And less like this:

    Settings* settings = document->settings();
    if (settings && EditingBehavior(settings->editingBehaviorType()).shouldFlashCaretNeonWhenTypingOjansName())

We could have Settings only know about the enum if we like, but that's not critical. What is critical is that the mapping from enum values to all the functions be in its own header and not mixed in with other stuff.

I don't even think all those functions need to be in a class. That style could be like this:

    Settings* settings = document->settings();
    if (settings && shouldFlashCaretNeonWhenTypingOjansName(settings->editingBehavior()))

Or this:

    if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))

[Ojan Vafai]

(In reply to comment #12)
>     if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())
>     if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))

Either one of these is fine with me. I prefer to avoid the other ones where we need to null-check settings. The latter seems simplest to implement, but I don't really care which of these two we do.

Also, can I add this as my own personal easter egg? :)

[Antonio Gomes]

Created attachment 57430 [details]
3rd simple design - feedback?

Summary:

* Implemented it so that call sites will look like the quoted code below:
>     if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())
>     if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))
* Added WebCore/editing/EditingBehavior.{cpp,h} so all editing behavior methods can will be grouped together on their own header.
* Implemented it so that Settings class only knows about the enum.
* Renamed the enum in Settings from EditingBehavior to EditingBehaviorType, and changed places in WebKit referring to it.

[Antonio Gomes]

Comment on attachment 57430 [details]
3rd simple design - feedback?

err ... wrong file :(, sorry for bug spamming

[Antonio Gomes]

Created attachment 57431 [details]
3rd simple design (right file) - feedback?

[Darin Adler]

Comment on attachment 57430 [details]
3rd simple design - feedback?

> +bool EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> +{
> +    if (Settings* settings = settingsForEditor())
> +        return settings->editingBehavior() != EditingWindowsBehavior;
> +
> +    return false;
> +}

We need to write a lot of these functions, so I'd like these to be as streamlined as possible. I suggest a helper function named type() that returns the editing behavior type and then:

    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return type() != EditingWindowsBehavior; }

And I'd like to see these in the header if possible, since we want these to be easy to read.

But I think what we actually want is this:

    class EditingBehavior {
    public:
        EditingBehavior(EditingBehaviorType type) : m_type(type) { }

        bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
 
    private:
        EditingBehaviorType m_type;
    }

There's no reason for EditingBehavior to have an Editor* in it.

[Antonio Gomes]

(In reply to comment #17)
> (From update of attachment 57430 [details])
> > +bool EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> > +{
> > +    if (Settings* settings = settingsForEditor())
> > +        return settings->editingBehavior() != EditingWindowsBehavior;
> > +
> > +    return false;
> > +}
> 
> We need to write a lot of these functions, so I'd like these to be as streamlined as possible. I suggest a helper function named type() that returns the editing behavior type and then:
> 
>     bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return type() != EditingWindowsBehavior; }
> 
> And I'd like to see these in the header if possible, since we want these to be easy to read.
> 
> But I think what we actually want is this:
> 
>     class EditingBehavior {
>     public:
>         EditingBehavior(EditingBehaviorType type) : m_type(type) { }
> 
>         bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
> 
>     private:
>         EditingBehaviorType m_type;
>     }
> 
> There's no reason for EditingBehavior to have an Editor* in it.

darin, I uploaded a wrong file. could you re-check the later, please? Or your comments remain the same for that new one?

[Darin Adler]

(In reply to comment #18)
> darin, I uploaded a wrong file. could you re-check the later, please? Or your comments remain the same for that new one?

The same comments apply to the new one, but about Settings* rather than Editor*.

[Darin Adler]

In Settings:

    void setEditingBehaviorType(EditingBehaviorType type) { m_editingBehaviorType = type; }
    EditingBehavior editingBehavior() const { return static_cast<EditingBehaviorType>(m_editingBehavior); }

Note that the getter returns EditingBehavior, not EditingBehaviorType. That's where the "magic" happens.

[Antonio Gomes]

Created attachment 57468 [details]
4th try - feedback?

Summary:
    * Renamed EditingBehavior enum in Settings to EditingBehaviorType
    * Added EditingBehavior class, living in WebCore/editing/EditingBehavior.h . It does not know about external classes but EditingBehaviorType
    * Settings now has:
        - a setter for the EditingBehaviorType: setEditingBehaviorType(EditingBehaviorType)
        - a getter for the EditinbeBehavior, based on the type: EditingBehavior editingBehavior()
    * Call sites in WebKit/ adjusted to work with the above changes.
    * Editor.cpp has a behavior() getter, relying on Settings' editingBehavior method.
    * Call sites will look like:
        if (frame->editor()->behavior().shouldThisDesignBeUsed_questionMark()))
            /// do xxx

[Darin Adler]

Comment on attachment 57468 [details]
4th try - feedback?

> +#ifndef EditingBehavior_h
> +#define EditingBehavior_h
> +
> +#include "Settings.h"

It seems  shame to have to have EditingBehavior.h include the whole Settings.h file just to get the definition of the EditingBehaviorType enum. I would put that enum into its own file.

> +    // Platform variant editing behavior methods.

 I would not call these "methods" and nor would I use the phrase them "platform variant". I would say something more like:

    // Individual functions for each case where we have more than one style of editing behavior.
    // Create a new function for any platform difference so we can control it here.

My comment isn't perfect either, but I think it may be a bit easier to understand.

> +} // namspace WebCore

Typo "namspace".

> +        EditingBehavior editingBehavior() const;

Since we have an Editor function that returns EditingBehavior, we could make this Settings function just return EditingBehaviorType and make it so that Settings doesn't know anything about EditingBehavior at all.

> --- a/WebCore/rendering/RenderBlock.cpp
> +++ b/WebCore/rendering/RenderBlock.cpp
> @@ -19,16 +19,17 @@
>   * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>   * Boston, MA 02110-1301, USA.
>   */
>  
>  #include "config.h"
>  #include "RenderBlock.h"
>  
>  #include "Document.h"
> +#include "EditingBehavior.h"

This include is not needed because Editor.h takes care of it already.

Otherwise this looks great to me.

[Antonio Gomes]

Created attachment 57495 [details]
patch v1 - functional

* Addresses Darin's previously requests for feedback{1,2,3,4}

Summary:

* EditingBehavior enum renamed to EditingBehaviorTypes and move out to editing/EditingBehaviorTypes.h
- Also done with changes needed in WebKit/ due to the enum change.

* EditingBehavior class added. It is not to be directly instantiated, but rather accessed via Editor::behavior() getter.
- Settings class will only know about the EditingBehaviorTypes enum, not about EditingBehavior class
- EditingBehavior will only know about the EditingBehaviorTypes enum, not about Settings or Editor.

* All existing Settings::defaultBehavior() == Editing{Mac,Windows}Behavior changed to call methods in EditingBehavior class - (please review the methods names, I am open to suggestions).
* Added new files to the build various sytems (hope I did not miss any).

[WebKit Review Bot]

Attachment 57495 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/EditingBehaviorTypes.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 57495 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2805024

[Antonio Gomes]

Created attachment 57502 [details]
patch v2

Same patch as patch 57495 (see comment #23 for a summary), but includes a fix for Chromium-linux build (in comment #25) and style nit in comment #24.

As for the Mac build, I can not see the error log, and it gets hard to fix, but I will fix at office tomorrow.

Apart from that, patch is ready for review.

[Antonio Gomes]

Created attachment 57540 [details]
patch v2.1 - same as v2, but builds on Mac

Same as patch 57502, but builds on Mac. See comment #26

[Antonio Gomes]

Created attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

err. I missed settings it as private header.

see comment #23 for summary.

[Darin Adler]

Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

Patch is great. What we need to work on are the names of the behaviors. For each one, we should write a plain language sentence or paragraph describing the behavior and then summarize that for the name of the function.

> +    bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { return m_type != EditingMacBehavior; }

I think this name is a little vague. What this controls is style-toggling behavior. It doesn't affect other "is style present" situations, such as when to check the "Bold" menu item in an application. Does it?

> +    bool shouldExtendSelectionFromEndPointAlways() const { return m_type != EditingMacBehavior; }

This name is too broad. It controls only shift-click behavior, not selection extension that's done with shift-arrow-keys for example.

> +    bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }

What does "extend for horizontal navigation" mean? This one needs work.

> +    bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }

If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?

[Ojan Vafai]

Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

WebCore/editing/Editor.cpp:104
 +      if (!m_frame || !m_frame->page() || !m_frame->settings())
Do you need to null check page here? Looks like m_frame and settings should be sufficient.

WebCore/editing/EditingBehavior.h:43
 +      bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }
typo: Keybard

[Antonio Gomes]

Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

Removing the review flag for now based on current not-so-good names patch comes up with.

I will work on each method naming by deeply understanding what exactly it tries to work around by looking at the original commits that introduced it.

help welcome.

[Darin Adler]

We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.

[Antonio Gomes]

(In reply to comment #32)
> We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.

Ok, Darin. I can prepare it later today (today and tomorrow are holidays in Brazil)

[Antonio Gomes]

> > We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.
> 
> Ok, Darin. I can prepare it later today (today and tomorrow are holidays in Brazil)

... finally back on it.

@darin: you did not commented about these ones:

bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { ... }
bool shouldCenterAlignWhenSelectionIsRevealed() const { ... }
bool shouldConsiderMouseSelectionAsDirectional() const { ... }

Do you these are good to go as-are? If so, we could start with them, and incrementally working on the missing ones.

[Antonio Gomes]

Created attachment 57916 [details]
patch v2.3 - part I

As per suggestion in comment #32, patch "v2.3 - part I" introduces the following methods to EditingBehavior.h:

bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { ... }
bool shouldCenterAlignWhenSelectionIsRevealed() const { ... }
bool shouldConsiderMouseSelectionAsDirectional() const { ... }

Follow up work will replace all remaining calls to Editing{Mac,Windows}Behavior by new functions in EditingBehavior class.

remaing ones, currently bad named are:

(these are close imo)
bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { ... }
bool shouldExtendSelectionFromEndPointAlways() const { ... }

(these need love)
bool shouldKeybardSelectionExtendForHorizontalNavigation() const { ... }
bool shouldGrowSelectionWhenExtendingToBoundary() const { ...}

[Antonio Gomes]

Comment on attachment 57916 [details]
patch v2.3 - part I

wrong patch

[Antonio Gomes]

Created attachment 57919 [details]
 patch v2.3 - part I   (right file) 

see comment #35 for summary

[Antonio Gomes]

Created attachment 58070 [details]
patch v2.3 - part I (agaisnt ToT)

Same as patch 57919 (patch v2.3 - part I (right file)), but rebased against ToT for the bots to build it.

Details: Patch did not build on EWS's bot because it had a conflit in WebCore/WebCore.xcodeproj/project.pbxproj

For summary see comment #35.

[Early Warning System Bot]

Attachment 58070 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3160141

[Antonio Gomes]

Created attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

Same as patch v2.3 (see comment #35), but now against ToT and fixed build issue in comment #39.

[Ojan Vafai]

Comment on attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

This looks good to me with the nit below fixed.

WebCore/editing/Editor.cpp:104
 +      if (!m_frame || !m_frame->page() || !m_frame->settings())
Do you need to null-check page here? m_frame and settings should be sufficient.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60816 might have broken GTK Linux 64-bit Debug

[Antonio Gomes]

Comment on attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

Clearing flags on attachment: 58081

Committed r60841: <http://trac.webkit.org/changeset/60841>

[Antonio Gomes]

Finally some time to back work on this! I will follow Darin's suggestion to follow up incrementally here, case by case.

> > +    bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { return m_type != EditingMacBehavior; }
> 
> I think this name is a little vague. What this controls is style-toggling behavior. It doesn't affect other "is style present" situations, such as when to check the "Bold" menu item in an application. Does it?

In fact, it is a possible situation, yes.

In QtWebKit we have something called 'WebAction' (see [1]), accessible through the QWebPage::WebAction enum. It does include some text specific styles including bold, italic, underline , subscript, superscriput, strikethrough.

[1] http://doc.qt.nokia.com/4.7-snapshot/qwebpage.html#WebAction-enum

It is possible app developers to connect a menu item to, say for instance, QWebPage::ToggleBold, like in the snippet below:

void MainWindow::editMenu()
{
  QToolBar* bar = addToolBar("Edit");
  bar->addAction(page()->action(QWebPage::ToggleBold));
  ...
}

It enables QtWebKit to enable/disable and to check/uncheck menu items in the application chrome side, according to the WebCore state. Same mechanism is applied to Edit->Paste|Copy|Cut menu items. If no text is in clipboard then Paste menu item will be disable. If no text selected in content, Copy will be disabled, and so on ...

Specifically about ToggleBold, suppose user makes a text selection with mouse. At some point, EditorClient::respondToChangedSelection will be called.

void EditorClientQt::respondToChangedSelection()
{
  ...
  m_page->d->updateEditorActions();
  ...
}

which will end up calling

QWebPagePrivate::updateAction(QWebPage::WebAction action)
void QWebPagePrivate::updateEditorActions()
{
  updateAction(QWebPage::Cut);
  updateAction(QWebPage::Copy);
  updateAction(QWebPage::Paste);
  ...
  updateAction(QWebPage::ToggleBold);
  ...
}

void QWebPagePrivate::updateAction(QWebPage::WebAction action)
{
 ...
 WebCore::FrameLoader *loader = mainFrame->d->frame->loader();
 WebCore::Editor *editor = page->focusController()->focusedOrMainFrame()->editor();

 bool enabled = a->isEnabled();
 bool checked = a->isChecked();

 switch (action) {
   case QWebPage::Back:
   ...
   default: {
     // see if it's an editor command
     const char* commandName = editorCommandForWebActions(action);

     // if it's an editor command, let it's logic determine state
     if (commandName) {
     Editor::Command command = editor->command(commandName);
     enabled = command.isEnabled();
     if (enabled)
       checked = command.state() != FalseTriState;
     else
       checked = false;
   }
   break;
 }

If the user open up the Edit menu item, "command.state()" will do the trick of saying if the menu item has to be enabled, checked, or not for the ToggleBold action.

[Antonio Gomes]

Created attachment 59894 [details]
patch v3 - part II

Patch addresses the situation described in comment #44.

[Antonio Gomes]

Comment on attachment 59894 [details]
patch v3 - part II

cq- so I can fix some bad English in "Patch makes situations that depend on this check like this to be controlled by the EditingBehavior class." in the ChangeLog.

supposing it gets r+, of course.

[Antonio Gomes]

Created attachment 60186 [details]
patch v3.1 - part II

Same as patch v3 - part II (attachment 59894 [details]), but fixed ChangeLog entry.

@Darin or Ojan, could you please review? (please read comment #44)

[Antonio Gomes]

(In reply to comment #47)
> Created an attachment (id=60186) [details]
> patch v3.1 - part II
> 
> Same as patch v3 - part II (attachment 59894 [details]), but fixed ChangeLog entry.
> 
> @Darin or Ojan, could you please review? (please read comment #44)

Ping review(?)

[Ojan Vafai]

Comment on attachment 60186 [details]
patch v3.1 - part II

>  static TriState stateStyle(Frame* frame, int propertyID, const char* desiredValue)
>  {
>      RefPtr<CSSMutableStyleDeclaration> style = CSSMutableStyleDeclaration::create();
> -    style->setProperty(propertyID, desiredValue);
> -    return frame->editor()->selectionHasStyle(style.get());
> +    style->setProperty(propertyID, desiredValue); // We need to add this style to pass it to selectionStartHasStyle / selectionHasStyle
> +
> +    TriState styleIsPresent;
> +    if (!frame->editor()->behavior().shouldConsiderStylePresentOnlyIfThroughoutTheSelection())
> +        styleIsPresent = frame->editor()->selectionStartHasStyle(style.get()) ? TrueTriState : FalseTriState;
> +    else
> +        styleIsPresent = frame->editor()->selectionHasStyle(style.get());
> +
> +    return styleIsPresent;

This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. I think Darin was just asking that the name of the EditingBehavior method be more specific. How about something like shouldToggleStyleBasedOnStartOfSelection?

[Antonio Gomes]

(In reply to comment #49)
> I think Darin was just asking that the name of the EditingBehavior method be more specific. How about something like shouldToggleStyleBasedOnStartOfSelection?

Thank you for the comments, Ojan!

What I tried to explain in comment #44 is the situation "consider text as styled if style is present in all text nodes of a selection" encloses the following cases:

1) the toggle style behavior. For that situation, shouldToggleStyleBasedOnStartOfSelection as a name it great.

- example: the BOLD button of a rich text editor.

2) the "what is style state" situation? For that one, the method name you suggested does not work very well.

- example: an application menu item that has its checked/unchecked status relying on the style considered as present or not.

What do you think?


> This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. 

Could you tell why you think it is wrong?

[Darin Adler]

I think Ojan’s main point is that first we want to refactor these settings and give them good names without changing behavior. Changing behavior should be handled separately.

[Ojan Vafai]

(In reply to comment #50)
> What I tried to explain in comment #44 is the situation "consider text as styled if style is present in all text nodes of a selection" encloses the following cases:

Doh. Sorry, I missed that whole set of comments.

On a side note, in the future, followup patches for a given issue should be done in new bugs. They new bug can point to the original, but it makes it easier to follow the relevant bits for the patch in question. In general, it should be one patch per bug.

> > This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. 
> 
> Could you tell why you think it is wrong?

When shouldConsiderStylePresentOnlyIfThroughoutTheSelection is true it's taking what used to be a tri-state and making it a boolean. Specifically, I think we'd return the wrong value in the following case:
<div id=foo contentEditable>foo<b>bar</b></div>
<script>
window.getSelection().selectAllChildren(foo);
console.log(document.queryCommandIndeterm('bold'));
</script>

That should log "true" to the console. With your patch, I think it would log "false". On the other hand, this patch fixes queryCommandState to return the correct value in this case. Currently it would return true for the above and should return false.

It would be unfortunate to fix one case and break the other. Can we do this as two separate patches?

1. Just change the current editingBehavior call. Call the method shouldToggleStyleBasedOnStartOfSelection, but also put a FIXME to use that method in stateStyle for the cases where it's used for queryCommandState.
2. Rename shouldToggleStyleBasedOnStartOfSelection to shouldConsiderStylePresentOnlyIfThroughoutTheSelection and use it in stateStyle, but not if it's being used to get a return value for queryCommandIndeterm. This is a bit complicated because right now both queryCommandState and queryCommandIndeterm end up calling the same method. So it would require a few more changes to EditorCommand.cpp.

[Antonio Gomes]

Created attachment 60921 [details]
(r62815,r=ojan) patch v3.2 - part II

As suggested by Ojan and Darin, keep only refactor work happening in this bug. No behavioral should happen here.

[Antonio Gomes]

Comment on attachment 60921 [details]
(r62815,r=ojan) patch v3.2 - part II

Clearing flags on attachment: 60921

Committed r62815: <http://trac.webkit.org/changeset/62815>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62815 might have broken Qt Linux Release

[Antonio Gomes]

(In reply to comment #55)
> http://trac.webkit.org/changeset/62815 might have broken Qt Linux Release

Fixed in r62815 . Bogus ! in the IF statement.

[Robert Hogan]

This broke editing/5195166-1.html on the Qt bot.

[Antonio Gomes]

(In reply to comment #57)
> This broke editing/5195166-1.html on the Qt bot.

robert, this breakage is related to http://trac.webkit.org/changeset/62816 not this bug.

[Antonio Gomes]

Given the amount of comments in this bug and that its main goal is also accomplished, lets consider it as FIXED, and move the work involving specific missing bits in follow up bugs.

I will file all of them now, and link to this one for the record.

Thank you Darin and Ojan.

[Antonio Gomes]

(In reply to comment #29)
> > +    bool shouldExtendSelectionFromEndPointAlways() const { return m_type != EditingMacBehavior; }
> 
> This name is too broad. It controls only shift-click behavior, not selection extension that's done with shift-arrow-keys for example.

bug 41977.

> > +    bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }
> 
> What does "extend for horizontal navigation" mean? This one needs work.

bug 41976.
 
> > +    bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }
> 
> If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?

bug 41975.

[Antonio Gomes]

(In reply to comment #52)
> 2. Rename shouldToggleStyleBasedOnStartOfSelection to shouldConsiderStylePresentOnlyIfThroughoutTheSelection and use it in stateStyle, but not if it's being used to get a return value for queryCommandIndeterm. This is a bit complicated because right now both queryCommandState and queryCommandIndeterm end up calling the same method. So it would require a few more changes to EditorCommand.cpp.

bug 41989.

And we're done here.