Andrew Comminos [:acomminos] Assignee
	Description • 9 years ago

Running mochitest dom/events/test/test_bug426082.html on GTK3 builds fails.

TEST-UNEXPECTED-FAIL | dom/events/test/test_bug426082.html | Moving the mouse down from the label should have unpressed the button.

	Andrew Comminos [:acomminos] Assignee
	Comment 1 • 9 years ago

This patch makes GTK3's GtkStateFlags more consistent with what is currently done on GTK2 (requiring hover for the button active state), fixing the test in this bug.

Thanks!

	Karl Tomlinson (:karlt)
	Comment 2 • 9 years ago

Comment on attachment 8608914 [details] [diff] [review]
Fix button focus state flags on GTK3

I doubt this is the right fix.
I suspect there are some widgets that should be active even when there is no hover.
It shouldn't be the drawing code applying a restriction that the widget is not active when not hovered.  That should be determined by the particular widget implementation.

Is it depressed or active that is set on the button when not expected?

The GTK2 code is quite different because the widget can have only one state.  With GTK3 there are different flags, and so different combinations are possible.

	Andrew Comminos [:acomminos] Assignee
	Comment 3 • 9 years ago

'active' is being set on buttons that are not currently being hovered over, which violates the expectation of dom/events/test/test_bug426082.html.

I agree though, moving the activity decision out of the drawing code makes a lot of sense. What are your thoughts on putting the check in nsNativeThemeGtk::GetGtkWidgetAndState?

Cocoa does something similar in nsNativeThemeCocoa.cpp when dealing with buttons:

> [cell setHighlighted:isActive &&
>                      inState.HasAllStates(NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER)];

	Andrew Comminos [:acomminos] Assignee
	Comment 5 • 9 years ago

This patch moves the button activity decision based on hover state to nsNativeThemeGTK::GetGtkWidgetAndState. Thanks!

	Karl Tomlinson (:karlt)
	Comment 6 • 9 years ago

Comment on attachment 8622475 [details] [diff] [review]
Make GTK button activity require hover.

>       }
> 
>+      if (aWidgetType == NS_THEME_BUTTON) {
>+        aState->active &= aState->inHover;
>+      }

Please make this "else if".

I think this is fine, but some other button widgets should also behave similarly.

	Andrew Comminos [:acomminos] Assignee
	Comment 7 • 9 years ago

Updated to cover more button types.

Try (GTK2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=074645cc5482

	Pulsebot
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/709775ec03a2

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/709775ec03a2