fix createKMessageBox focus widget inconsistency
ClosedPublic

Authored by emateli on Sep 14 2017, 9:13 PM.

Details

Summary

When creating dialogs via createKMessageBox in case the QDialogButtonBox parameter is parentless, the focus shifts from the left most button to any of the following in this order:

  1. Message Label (if the AllowLink flag is present)
  2. The string list (if it is not empty)
  3. Checkbox confirmation (If present)
  4. Left most action button

Moving the call of setParent of the QDialogButtonBox before any of the above mentioned widgets is created fixes this inconsistency.

Test Plan

A use case where an application sets a default button but the checkbox is still selected instead is Dolphin.

  1. Open more than 1 tab in Dolphin
  2. Close Dolphin

Expected: The quit button is focused
Actual: Checkbox "Do not ask again" is focused

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
emateli created this revision.Sep 14 2017, 9:13 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 14 2017, 9:13 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
emateli edited the summary of this revision. (Show Details)Sep 14 2017, 9:28 PM
emateli edited the test plan for this revision. (Show Details)
emateli added a reviewer: Frameworks.
aacid added a subscriber: aacid.Sep 14 2017, 9:51 PM

This should come with an autotest

@aacid just to make clear, you're saying to add another case to the testMessageBox on kmessageboxtest.cpp right?

aacid added a comment.Oct 3 2017, 9:41 PM

@aacid just to make clear, you're saying to add another case to the testMessageBox on kmessageboxtest.cpp right?

No, look in the autotest folder not in the test folder.

emateli updated this revision to Diff 20497.Oct 8 2017, 8:26 PM

Added autotests

@aacid, all good now?

ngraham accepted this revision.Oct 22 2017, 9:59 PM

+1, this works great for me. Any other objections, or can I land this?

This revision is now accepted and ready to land.Oct 22 2017, 9:59 PM
aacid added a comment.Oct 23 2017, 9:27 PM

Sorry but after reading what the patch does i'm not sure this makes sense.

Default button just means "the button that is triggered when pressing Enter", it has nothing to do with "this is the button that should have focus".

Have you talked with the usability people about this?

There actually is a bug being fixed here in that currently many buttons marked default aren't actually getting made default buttons in that they turn blue and pressing enter will click them. This fixes that.

In my opinion a "Default" widget, should also have focus. Makes little sense to have some widget focused and then pressing enter to trigger the action of another one.
By focusing, the default action is also made visible and allows to invoke the it action by pressing space bar. And no, I haven't spoken to the usability team. What is the team name so we can tag them here perhaps?

After running some tests even when manually setting some other action to default, enter always invokes the same action. Eg: in the case of a Yes, No, Cancel dialog, the yes will always be called regardless.

aacid added a comment.Oct 23 2017, 9:57 PM

Sorry to be blunt, but your opinion doesn't matter.

Here is the definition of the default property for a button.

http://doc.qt.io/qt-5/qpushbutton.html#default-prop

Nowhere in it says "this is the button that should have focus when the dialog pops up".

aacid requested changes to this revision.Oct 23 2017, 9:58 PM

Marking as request changes since i'd like someone with UI expertise to confirm this is what we want since from the code point of view default = focus is not what the default property for buttons means.

This revision now requires changes to proceed.Oct 23 2017, 9:58 PM

There actually is a bug being fixed here in that currently many buttons marked default aren't actually getting made default buttons in that they turn blue and pressing enter will click them. This fixes that.

I don't understand this sentence, can you rewrite it and/or give an example?

ngraham added a comment.EditedOct 24 2017, 2:09 AM

@aacid sorry to be unclear.

To most people, the default button is the one that's visually distinct. In Breeze, that generally is the one with a blue background.

Without this patch, the *actual* default button is virtually indistinguishable from other buttons when it doesn't have focus. It looks like this, with only a slightly lighter background than the other buttons:

With the patch, the default button gets focus, so it looks like a default button should (IMHO):

IMHO it's more important to put the focus where it will cause the button to look correct than on an unrelated UI element. However since correct appearance rather than focus status is what's visually important here, I would be okay rejecting and abandoning this patch if we can find an alternate route to this goal. For example: instead changing Breeze to make sure that default buttons are always colored blue, regardless of focus status (which should then be indicated some other way).

@aacid the issue at hand is that in this particular scenario the default button more or less equals to the one that has focus.

There are only 3 types of widgets that can be created here: buttons, a list or a check box.

You can't interact with the list so it is ruled out for being focused. The check box acts as a confirmation and thus is a secondary element which you also do not want to toggle by mistake, so it rests upon the buttons to have it.
If any button is to have focus then it might as well be the default one, giving it to another button will break the enter button since the focused button's handler will be invoked over the default's.
This also has the added benefit of allowing to invoke the default action via space bar.

ngraham added a reviewer: VDG.Oct 24 2017, 2:01 PM
aacid added a comment.Oct 24 2017, 9:39 PM

Without this patch, the *actual* default button is virtually indistinguishable from other buttons when it doesn't have focus. It looks like this, with only a slightly lighter background than the other buttons:

This is a bug of the breeze style, go fix it there.

aacid added a comment.Oct 24 2017, 9:44 PM

You can't interact with the list so it is ruled out for being focused. The check box acts as a confirmation and thus is a secondary element which you also do not want to toggle by mistake, so it rests upon the buttons to have it.

This makes some sense, but aren't you are breaking existing code?

see detailedErrorInternal, detailedSorryInternal, they call setFocus on a button which you are probably then overriding?

In D7828#159511, @aacid wrote:

Without this patch, the *actual* default button is virtually indistinguishable from other buttons when it doesn't have focus. It looks like this, with only a slightly lighter background than the other buttons:

This is a bug of the breeze style, go fix it there.

Sure: https://bugs.kde.org/show_bug.cgi?id=386158

If the Breeze folks indicate that their preferred solution is to implement this patch, can we go for it?

romangg added a subscriber: romangg.EditedOct 25 2017, 1:00 AM

Hi Emirald,

first let me tell you that I totally get where you're coming from. To be honest the selected checkbox in this specific dialog of Dolphin has annoyed me already for a long time. So thank you for looking into this.

But we have to dissect the situation a bit more: A button with focus and the default button in a KMessageBox are two different things per definition as Albert pointed out.

That said I can only think of one sensible case where this distinction makes sense: When no element has focus, but you still want a default action to trigger on Enter. In any case having focus on some control element but then pressing Enter triggering an action related to some other button doesn't seem like good Ui to me. So this sounds similar to what you say.

But in the end it should be up to the app developer to decide if the focus and default action element are the same one or different ones in his KMessageBox. I would still advise him in general to not make them different, but don't forget that your change is a change in Frameworks restricting it to our presumed best practice for all apps using a KMessageBox. And there might be corner cases we haven't thought about, where different focus and default action make sense.

So I think this patch shouldn't go into Frameworks. That said the focused checkbox in Dolphin should go away (as I said in the beginning it annoyed me already for a longer time). The rationale behind this was maybe, so a user could press Space for activating the checkbox and Enter for triggering the default action, but checking a focused checkbox with Space and not Enter isn't obvious and this checkbox also isn't something a user totally needs to have one button keyboard access to. So just set in this specific Dolphin dialog the focus item to the Ok button. By the way this is not something totally new to Dolphin. The "Confirm delete" dialog has its default button focused.

abetts added a subscriber: abetts.Oct 25 2017, 1:45 AM

I am leaning toward implementing the change. The reason being that the user gets a duplicate indication of focus when likely they might feel confused, I have been at times too, by not knowing clearly where the highlight is. If you have two highlights, how do you know what's going to be activated when hitting enter?

If you have two highlights, how do you know what's going to be activated when hitting enter?

My thoughts exactly.

emateli added a comment.EditedOct 25 2017, 8:31 AM

Hey @subdiff, thanks for your input on this. Whether this patch goes in or not, I still think that this "odd" behaviour is something that the frameworks should fix or change rather than relying on the developer of each application to do this.

However I never expected this to stir up such a conversation since I thought this was the normal behaviour in Plasma, given that all(?) the other dialogs created by the framework have the default button focused (See: Konsole close dialog w/ multiple tabs open (the exact same dialog as Dolphin's), or Konversation after having joined a channel. )

Since these functions internally all rely on createKMessageBox and work properly so to say, I dug a bit deeper into this and found out the following why Dolphin's close dialog is the odd one out: If no parent window is specified when calling this function then the checkbox will be focused, if one is passed then the focus shifts onto the buttons.

Current call: QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | QDialogButtonBox::Cancel);
"Fixed" call: QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | QDialogButtonBox::Cancel, dialog);

This leads me to believe that the checkbox having the focus is an irregularity, whether one would consider it a bug is a matter of prespective. But i would expect the frameworks to be consistent in what it does and whether the dialog has a parent or not should not affect what widget is focused.

But just like @aacid mentioned, we should not look to break existing code, so given the new information on why this occurs, then perhaps a new implementation might be needed or simply a fix for this change in behaviour when a parent widget is present or not.

aacid added a comment.Oct 25 2017, 7:20 PM

If you have two highlights, how do you know what's going to be activated when hitting enter?

My thoughts exactly.

But it does not really solve that, once you press tab and then you still have the same problem, right?

Once you press tab the focus moves elsewhere and hitting return or enter will press whatever's selected rather than the original default button. So really the concept of a "Default Button" in our world seems to mostly be synonymous with "the button that receives focus by default." In this case, then there should only be one focus, and that focus should be on whatever is marked as a default button. Makes sense to me.

aacid added a comment.Oct 25 2017, 7:41 PM

Once you press tab the focus moves elsewhere and hitting return or enter will press whatever's selected rather than the original default button. So really the concept of a "Default Button" in our world seems to mostly be synonymous with "the button that receives focus by default." In this case, then there should only be one focus, and that focus should be on whatever is marked as a default button. Makes sense to me.

Hmmm, that's not really what it happens, is it?
otherwise "i am focused on the checkbox but press enter and the button gets pressed" would never happen?

In D7828#160028, @aacid wrote:

Once you press tab the focus moves elsewhere and hitting return or enter will press whatever's selected rather than the original default button. So really the concept of a "Default Button" in our world seems to mostly be synonymous with "the button that receives focus by default." In this case, then there should only be one focus, and that focus should be on whatever is marked as a default button. Makes sense to me.

Hmmm, that's not really what it happens, is it?
otherwise "i am focused on the checkbox but press enter and the button gets pressed" would never happen?

It seems that regardless, the condition with this Dolphin dialog box where the checkbox receives focus but another buttin is marked as Default is actually unintentional, and is a bug that should be fixed.

Try this in Kate: Open a new document, type some stuff, and close it. In the resulting dialog box, he "Save" button is focused by default, but if you hit tab or the right arrow key, the focus will shift to another button and then hitting return or enter will press that button. This is a really nice feature.

In other words, it seems intentional that the button marked default will always receive focus.

emateli added a comment.EditedOct 25 2017, 8:00 PM

I think this is getting a bit out of hand here. Please try to read my last message where I explain why I decided to submit this patch and why I think it's a bug (the whole parent widget thing).

Given the new information on why this occurs the whole focus by force thing might not be optimal (although I don't see it as being wrong either), so we can check for a solution that perhaps simply fixes the inconsistency when being called with a parent and without one.
This could be easily solvable in Dolphin's repository where this would need only one additional parameter to the createKMessageBox call, but I still thing it belongs to be fixed in the frameworks.

Edit:

aacid added a comment.Oct 25 2017, 8:23 PM

In other words, it seems intentional that the button marked default will always receive focus.

No, open kate->settings->configure kate

Ok is the default button and it is not supposed to have the focus, default == should have focus is simply not what default means.

rkflx added a subscriber: rkflx.Oct 25 2017, 10:30 PM

In general, having a default button while setting the initial focus elsewhere is mainly useful for larger dialogs like our various standard configuration dialogs, where this provides a way to accept without tabbing forever. For smaller dialogs like a messagebox this is not that important, but at least consistent. For messageboxes my general expectation is that the default button should also have focus to more clearly guide the user in the best decision regarding what button choose.

That said, I agree with @subdiff to at least allow deviations from this, while at the same time fixing Dolphin. BTW, Dolphin's inconsistency is the reason why I mentioned this Differential in T7022.

a fix for this change in behaviour when a parent widget is present or not

This sounds reasonable and would be great if this fixed Dolphin. There is another odd behaviour in Dolphin's tab close confirmation dialog compared to e.g. Konsole's: First the default button is Quit, after switching focus around with in every possible direction suddenly Close is default. Maybe this is related to not specifying a parent window and could help solving this?

emateli updated this revision to Diff 21453.Oct 27 2017, 7:34 PM

Well, the diff of this patch took a sizeable reduction.

I took a second look at this and here is what happened in the end and what caused the inconsistency with Dolphin.

The issue was that the QDialogButtonBox argument, which contains the buttons needs to have a not null parent argument. If it does, then everything works as expected, if it doesn't (which Dolphin does not suppliy) then the focus is all messed up and will be given in order to the widgets created that can be focused.

From what I dug out the order seems to be:

  1. Label (if the AllowLink flag is present)
  2. The string list
  3. Checkbox
  4. Left most action button

Moving the call of setParent before any of the above mentioned widgets is created seems to fix this (I built this system-wide so all the KDE apps running can use it and no issues so far, although it's only been a couple of hours)

If this turns out to be okay, I'll edit the summary and title later, since it is re-purposed in a way by not involving widget focus change.

Does this mean that apps should always pass a parent? (if that's the case, we should mention it in the KMessageBox documentation)

rkflx accepted this revision.Oct 28 2017, 9:13 PM

If this turns out to be okay, I'll edit the summary and title later

Playing around with Dolphin's usage of createKMessageBox, I made the following observations:

  • Focus: Before on checkbox, now on first button.
  • Default: Before on some random button (changing with app restart), now consistently on first button.
  • setFocus to a different button was broken and is still broken.
  • setDefault to a different button was broken and is still broken.

This is a great improvement and already fits Dolphin's case. As this does not regress any other usage (remember, this was broken already), I would be in favour of committing this now (after adapting the commit message).

@aacid As you "requested changes", would you be okay with that?

That said, a followup patch would be marvelous (add autotests, fix setFocus and setDefault).

Does this mean that apps should always pass a parent? (if that's the case, we should mention it in the KMessageBox documentation)

If at all, apps should never pass a parent, because it gets overwritten anyway. @emateli is just moving the code for this around, not changing it. But as the docs already state "This function will take care of connecting to it.", adding something about a parent will probably only cause more confusion.

emateli retitled this revision from createKMessageBox tries to focus a default button when available to fix createKMessageBox focus button inconsistency.Oct 30 2017, 8:36 PM
emateli edited the summary of this revision. (Show Details)
emateli retitled this revision from fix createKMessageBox focus button inconsistency to fix createKMessageBox focus widget inconsistency.
aacid resigned from this revision.Oct 30 2017, 8:53 PM

This change is much less invasive than before, i'm not giving a +1 since i don't immediately understand why this fixes things and since i don't have time to investigate it, i'm removing my -1

This revision is now accepted and ready to land.Oct 30 2017, 8:53 PM
ngraham set the repository for this revision to R236 KWidgetsAddons.Oct 30 2017, 9:06 PM

Thanks @aacid. Any remaining objections to landing this?

Ping @subdiff @abetts does this iteration work for you guys? It's marked as ready to land but I feel that we should get an overall opinion on this.

romangg accepted this revision.Nov 2 2017, 4:54 PM

Looked at the code and I think @rkflx explained the situation quite well (if we should force passing a parent or not - better - for the QDialogButtonBox).

I think we've got enough thumbs up. I'm gonna land this.

This revision was automatically updated to reflect the committed changes.