1428297 - Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement, P3)

Description

[Chih-Hsuan Kuo [:tommykuo]]

To render the dropdown menu in the content process, we should get rid of using widget/view to show that.

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review commit: https://reviewboard.mozilla.org/r/210774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210774/

Comment 2

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210774/diff/1-2/

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210774/diff/2-3/

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

Review commit: https://reviewboard.mozilla.org/r/211616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211616/

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8941315 [details]
Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211616/diff/1-2/

Comment 6

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210774/diff/3-4/

Comment 7

[Chih-Hsuan Kuo [:tommykuo]]

It seems that the patches cannot pass the a11y test.

Comment 8

[Chih-Hsuan Kuo [:tommykuo]]

Hi Jamie,

I found my part 1 patch made the a11y test case fail[1]. However, I think my patch would only work when the pref is set to true, so it would do nothing by default. Then I tried to apply the patch[2] that you provide, and it seems to pass the test case. Do you know the reason? I don't think my patch will affect the original code. Thanks!

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc770fb282d8e35aa300dc2acd012dc619987a0&selectedJob=155240302
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1421229#c25

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8941315 [details]
Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211616/diff/2-3/

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210774/diff/4-5/

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

Oh... I found the problem. There is a missing `!` in my patch.

Comment 12

[Mats Palmgren (inactive)]

Preferences::GetBool() is a bit slow.  I think you should make a nsLayoutUtils
function to lookup that pref and use Preferences::AddBoolVarCache in that function.
See for example:
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/layout/base/nsLayoutUtils.cpp#624

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1428297 - (Part 1) Add an nsLayoutUtils function to check whether content-select is enabled

Review commit: https://reviewboard.mozilla.org/r/211938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211938/

Comment 14

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8941315 [details]
Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211616/diff/3-4/

Comment 15

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210774/diff/5-6/

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8941696 [details]
Bug 1428297 - (Part 1) Add an nsLayoutUtils function to check whether content-select is enabled

https://reviewboard.mozilla.org/r/211938/#review217842

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8941315 [details]
Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.

https://reviewboard.mozilla.org/r/211616/#review217912

::: layout/base/nsLayoutUtils.cpp:7437
(Diff revision 4)
> -  if (frameType == LayoutFrameType::ListControl) {
> +  if (!nsLayoutUtils::IsContentSelectEnabled() &&
> +      frameType == LayoutFrameType::ListControl) {

Just to be clear for the record, this makes nsLayoutUtils::IsPopup()
return false for the dropdown frame.  This seems a little odd,
but if you think this is the right way to go it's fine with me.
I haven't analyzed in detail how the IsPopup() is used - I assume
you did and determined it's fine.

::: layout/forms/nsListControlFrame.cpp:978
(Diff revision 4)
> -  if (IsInDropDownMode()) {
> +  if (!nsLayoutUtils::IsContentSelectEnabled() &&
> +      IsInDropDownMode()) {
> +    // TODO(kuoe0) Remove the following code when content-select is enabled.
>      AddStateBits(NS_FRAME_IN_POPUP);

Ditto - it seems a bit odd to not mark the dropdown frame NS_FRAME_IN_POPUP, but I'll assume you checked the uses and determined it's fine.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8940576 [details]
Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.

https://reviewboard.mozilla.org/r/210774/#review217916

Comment 19

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Mats Palmgren (:mats) from comment #17)
> Just to be clear for the record, this makes nsLayoutUtils::IsPopup()
> return false for the dropdown frame.  This seems a little odd,
> but if you think this is the right way to go it's fine with me.
> I haven't analyzed in detail how the IsPopup() is used - I assume
> you did and determined it's fine.
> 
> Ditto - it seems a bit odd to not mark the dropdown frame NS_FRAME_IN_POPUP,
> but I'll assume you checked the uses and determined it's fine.

According to my analysis, I think the term, 'Popup', in Firefox means the pop-up windows that are using widgets to show. So, I think we don't need to set `NS_FRAME_IN_POPUP` and return true for `IsPopup()` when content-select is enabled.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4334dcb44c57
(Part 1) Add an nsLayoutUtils function to check whether content-select is enabled r=mats
https://hg.mozilla.org/integration/autoland/rev/6ed02115d2c6
(Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled. r=mats
https://hg.mozilla.org/integration/autoland/rev/46dd32e1f0fa
(Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled. r=mats

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1435b54875f555186ac934a13bd6a5a26d1b4432

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4334dcb44c57
https://hg.mozilla.org/mozilla-central/rev/6ed02115d2c6
https://hg.mozilla.org/mozilla-central/rev/46dd32e1f0fa