Closed Bug 1428297 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

Details

Attachments

(3 files)

Bug 1428297 - (Part 3) Disable the assertion that don't allow to call ShowDropDown() in the content process when content-select is enabled.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1428297 - (Part 2) Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1428297 - (Part 1) Add an nsLayoutUtils function to check whether content-select is enabled
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
To render the dropdown menu in the content process, we should get rid of using widget/view to show that.
Attachment #8940576 - Flags: review?(mats)
Component: Layout → Layout: Form Controls
Priority: -- → P3
Summary: Get rid of using widget/view to show the dropdown menu when content-select is enabled. → Ignore the widget/view usage that is used to show the dropdown menu when content-select is enabled.
Attachment #8941315 - Flags: review?(mats)
It seems that the patches cannot pass the a11y test.
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
Flags: needinfo?(jteh)
Attachment #8940576 - Flags: review?(mats)
Attachment #8941315 - Flags: review?(mats)
Oh... I found the problem. There is a missing `!` in my patch.
Flags: needinfo?(jteh)
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
Attachment #8941696 - Flags: review?(mats)
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
Attachment #8941696 - Flags: review?(mats) → review+
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.
Attachment #8941315 - Flags: review?(mats) → review+
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
Attachment #8940576 - Flags: review?(mats) → review+
(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.
Pushed by tokuo@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: