Closed Bug 1457719 Opened 6 years ago Closed 6 years ago

Remove the "autorepeatbutton" element

Categories

(Core :: XUL, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(4 files)

The "autorepeatbutton" element is only used by the base "arrowscrollbox" binding, and it can be replaced with a "toolbarbutton" like in the derived binding that handles the "clicktoscroll" case.
Attachment #8971820 - Flags: review?(enndeakin)
Comment on attachment 8971821 [details]
Bug 1457719 - Part 4 - Remove the "autorepeatbutton" element.

https://reviewboard.mozilla.org/r/240564/#review246558

::: layout/xul/nsButtonBoxFrame.cpp:211
(Diff revision 1)
>                                           nsGkAtoms::_true, eCaseMatters))
>      return;
>  
> -  // Execute the oncommand event handler.
> -  bool isShift = false;
> -  bool isControl = false;
> +  // Have the content handle the event, propagating it according to normal DOM rules.
> +  nsCOMPtr<nsIPresShell> shell = PresContext()->GetPresShell();
> +  if (!shell)

Curlies around if body, please.
Attachment #8971821 - Flags: review?(bzbarsky) → review+
Comment on attachment 8971820 [details]
Bug 1457719 - Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding.

https://reviewboard.mozilla.org/r/240562/#review248562

::: toolkit/content/widgets/scrollbox.xml:37
(Diff revision 1)
>      </implementation>
>    </binding>
>  
>    <binding id="arrowscrollbox" extends="chrome://global/content/bindings/scrollbox.xml#scrollbox-base">
>      <content>
> -      <xul:autorepeatbutton class="autorepeatbutton-up"
> +      <xul:toolbarbutton class="scrollbutton-up"

Does using scrollbutton-up and scrollbutton-down conflict with the rules for scrollbar buttons? I don't think I can review the style changes with any confidence.

::: toolkit/content/widgets/scrollbox.xml:349
(Diff revision 1)
> +            this._scrollTimer.cancel();
>  
> -          event.stopPropagation();
> +          let callback = () => this.scrollByPixels(this.scrollIncrement * index);
> +
> +          // Use the same REPEAT_DELAY as "nsRepeatService.h".
> +          let scrollDelay = /Mac/.test(navigator.platform) ? 25 : 50;

The original algorithm uses a delay of 250ms for the initial iteration.
Attachment #8971820 - Flags: review?(enndeakin)
(In reply to Neil Deakin from comment #4)
> Does using scrollbutton-up and scrollbutton-down conflict with the rules for
> scrollbar buttons? I don't think I can review the style changes with any
> confidence.

These class names seem only to be used in the context of the arrowscrollbox binding:

https://dxr.mozilla.org/mozilla-central/search?q=scrollbutton-up&redirect=false

Do you have nay further concerns? Do you have anyone in mind that could review the style changes?

> The original algorithm uses a delay of 250ms for the initial iteration.

I suspect this may be an artifact of using nsRepeatService. If this was designed to provide a grace period when the mouse is moved quickly over the arrow, I tested the current behavior on Windows and the delay is too short to be effective. I also tested the native menus on Mac and I can't see this delay, it seems to me that scrolling starts immediately as soon as the mouse moves from the nearby menu item to the arrow. I haven't tested Windows at the moment because I can't think of a native menu that has arrow scrolling, but I suspect it would be similar. Can we consider removing this initial delay to make the behavior more similar to the native one?
Flags: needinfo?(enndeakin)
(In reply to :Paolo Amadini from comment #5)
> Do you have nay further concerns? Do you have anyone in mind that could
> review the style changes?
> 

I don't know. I usually ask Dao to review style changes.


> I suspect this may be an artifact of using nsRepeatService. If this was
> designed to provide a grace period when the mouse is moved quickly over the
> arrow, I tested the current behavior on Windows and the delay is too short
> to be effective. I also tested the native menus on Mac and I can't see this
> delay, it seems to me that scrolling starts immediately as soon as the mouse
> moves from the nearby menu item to the arrow. I haven't tested Windows at
> the moment because I can't think of a native menu that has arrow scrolling,
> but I suspect it would be similar. Can we consider removing this initial
> delay to make the behavior more similar to the native one?

OK, then that is fine.
Flags: needinfo?(enndeakin)
Comment on attachment 8971820 [details]
Bug 1457719 - Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding.

Dão, can you take a look at the style changes here?
Attachment #8971820 - Flags: review?(dao+bmo)
Comment on attachment 8971820 [details]
Bug 1457719 - Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding.

https://reviewboard.mozilla.org/r/240562/#review252076

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 1)
> -  display: none;
> -}
> -
> -#appMenu-popup > arrowscrollbox > scrollbox {
> -  overflow: visible;
> -}

Why are you removing this?

::: toolkit/content/widgets/scrollbox.xml:344
(Diff revision 1)
> -            dir *= -1;
> +            index *= -1;
>  
> -          this.scrollByPixels(this.scrollIncrement * dir);
> +          if (!this._scrollTimer)
> +            this._scrollTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +          else
> +            this._scrollTimer.cancel();

nit: curly brackets

::: toolkit/content/widgets/scrollbox.xml:349
(Diff revision 1)
> +            this._scrollTimer.cancel();
>  
> -          event.stopPropagation();
> +          let callback = () => this.scrollByPixels(this.scrollIncrement * index);
> +
> +          // Use the same REPEAT_DELAY as "nsRepeatService.h".
> +          let scrollDelay = /Mac/.test(navigator.platform) ? 25 : 50;

use AppConstants.platform?
Attachment #8971820 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #8)
> > -#appMenu-popup > arrowscrollbox > scrollbox {
> > -  overflow: visible;
> > -}
> 
> Why are you removing this?

The main menu popup is an arrowpanel now, so I don't think either this selector or the one for "autorepeatbutton" above actually match. This may be a leftover from the earliest implementation of the application menu.

> ::: toolkit/content/widgets/scrollbox.xml:349
> > +          let scrollDelay = /Mac/.test(navigator.platform) ? 25 : 50;
> 
> use AppConstants.platform?

I'd have to import the module each time the binding is constructed, I believe this is why the other bindings in the folder use this alternative method.
Attachment #8971820 - Flags: review?(dao+bmo)
Comment on attachment 8971820 [details]
Bug 1457719 - Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding.

https://reviewboard.mozilla.org/r/240562/#review252486

::: toolkit/themes/linux/global/scrollbox.css:29
(Diff revision 2)
>    display: none;
>  }
>  
> -autorepeatbutton {
> +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-up,
> +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-down {
> +  -moz-appearance: none;

This doesn't seem to work, I'm getting the toolbarbutton appearance in overflowing menus with this patch on Linux.
Attachment #8971820 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #12)
> > +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-up,
> > +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-down {
> > +  -moz-appearance: none;
> 
> This doesn't seem to work, I'm getting the toolbarbutton appearance in
> overflowing menus with this patch on Linux.

Thanks for catching this! This is quite likely a regression from bug 1458584, that landed yesterday, and would now affect this patch. Since "scrollbox.css" became a UA sheet, its rules have always less priority than other files, including other XBL stylesheets like "toolbarbutton.css".

Converting all the XBL stylesheets to UA stylesheets at once might mitigate issues like this. The test build in bug 1463820 is investigating the effort involved, compared to adding to the platform a functionality equivalent to the XBL cascade level. It seems that the strategy of migrating component stylesheets one by one is not proving effective.

Brian, I'm not sure how you're tracking bugs in this bucket, you may want to set a bug dependency.
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #13)
> (In reply to Dão Gottwald [::dao] from comment #12)
> > > +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-up,
> > > +arrowscrollbox:not([clicktoscroll="true"]) > .scrollbutton-down {
> > > +  -moz-appearance: none;
> > 
> > This doesn't seem to work, I'm getting the toolbarbutton appearance in
> > overflowing menus with this patch on Linux.
> 
> Thanks for catching this! This is quite likely a regression from bug
> 1458584, that landed yesterday, and would now affect this patch. Since
> "scrollbox.css" became a UA sheet, its rules have always less priority than
> other files, including other XBL stylesheets like "toolbarbutton.css".
> 
> Converting all the XBL stylesheets to UA stylesheets at once might mitigate
> issues like this. The test build in bug 1463820 is investigating the effort
> involved, compared to adding to the platform a functionality equivalent to
> the XBL cascade level. It seems that the strategy of migrating component
> stylesheets one by one is not proving effective.
> 
> Brian, I'm not sure how you're tracking bugs in this bucket, you may want to
> set a bug dependency.

Do you want to move scrollbox.css back to a XBL sheet in the meantime? We could copy the <resources> stylesheet across to the bindings that used to inherit from scrollbox-base to avoid re-adding the base binding.
Flags: needinfo?(bgrinstead)
Comment on attachment 8980565 [details]
Bug 1457719 - Part 1 - Restore "scrollbox.css" as a XBL stylesheet.

https://reviewboard.mozilla.org/r/246714/#review253478

::: toolkit/content/widgets/scrollbox.xml:15
(Diff revision 1)
>     xmlns:xbl="http://www.mozilla.org/xbl">
>  
>    <binding id="scrollbox" extends="chrome://global/content/bindings/general.xml#basecontrol">
> +    <resources>
> +      <stylesheet src="chrome://global/skin/scrollbox.css"/>
> +    </resources>

We don't seem to need the stylesheet in this binding.
Attachment #8980565 - Flags: review?(dao+bmo) → review+
Comment on attachment 8971820 [details]
Bug 1457719 - Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding.

https://reviewboard.mozilla.org/r/240562/#review253480
Attachment #8971820 - Flags: review?(dao+bmo) → review+
Okay, it took a while to debug this, but long story short the "layout/reftests/bugs/508816-1.xul" failure on Linux is related to the "arrowscrollbox" binding being unable to work properly in unprivileged contexts. This appears to be the case since at least the first Mercurial revision.

The reftest currently succeeds because the pre-existing malfunctions result in two identical renderings in the reference and test files, but after this change the renderings differ.

To make the "arrowscrollbox" binding work in unprivileged contexts, we would have to provide a default for any "about:config" preference, and use setInterval instead of the nsITimer interface. If we don't care about running bindings in unprivileged pages, we'd probably have to make the XUL reftests privileged, so the XUL bindings loaded there will have access to Components.

Brian, is running in unprivileged documents something we want to support, in particular for the final state, where most XUL elements are implemented with Custom Elements? If I understand correctly, the infrastructure we're building for loading MozXULElement requires privileges, for example to load scripts.
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #21)
> To make the "arrowscrollbox" binding work in unprivileged contexts, we would
> have to provide a default for any "about:config" preference, and use
> setInterval instead of the nsITimer interface. If we don't care about
> running bindings in unprivileged pages, we'd probably have to make the XUL
> reftests privileged, so the XUL bindings loaded there will have access to
> Components.

Making the XUL reftests run privileged makes sense to me, since we don't generally expose XUL to content. Especially since it sounds like we are currently only passing this test because both the reference and test is broken. Is switching to chrome privileged for these tests something that's relatively easy to change?
 
> Brian, is running in unprivileged documents something we want to support, in
> particular for the final state, where most XUL elements are implemented with
> Custom Elements? If I understand correctly, the infrastructure we're
> building for loading MozXULElement requires privileges, for example to load
> scripts.

It would be great if we could support (at least some of) our elements in an unprivileged document, but that's a long way off. There's a prerequisite to not use XUL, and then on top of that to move away from chrome APIs (or provide a shimmed content-accessible version of them). I wouldn't worry about it for now.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #22)
> Is switching to chrome privileged for these tests something that's
> relatively easy to change?

I'm thinking we might just need to load the tests from "chrome:" instead of "file:". I'll try and see how easy it is to do that.
See Also: → 1465457
I filed bug 1465457 for loading XUL reftests in a privileged context, which may not be too difficult but not too easy either.

In the meantime I also found bug 1375012 which may be related. I have a tentative patch for the latter that I'm running on the tryserver, which doesn't fix the underlying issue completely, but may be able to unblock landing this bug without waiting for bug 1465457.
Depends on: 1375012
So, the patch in progress bug 1465730 fixes the issue with the failing test by waiting for a layout flush, which depends on running the test with privileges. So I think there are now two main ways of unblocking this bug:

1. Disable the test on all platforms, keeping bug 1465730 (or the original bug 1375012) open to re-enable the test.

We can re-enable the test once bug 1465457 is fixed, and since we are not constrained by time, we can choose any of the approaches listed in bug 1465457 comment 5, including running all XUL tests with privileges.

2. Fix bug 1465457 to run the test with privileges.

The only way to do this in a reasonable time is to add a reftest manifest directive in bug 1465457 to load just a specific test with privileges. The test would have to be excluded from running on Android where tests are loaded over HTTP and the "chrome:" URL mapping doesn't work out of the box.

I'd rather we went with the first option, since we have a clear way forward for re-enabling the test now. Dão, do you think this would work?
Flags: needinfo?(dao+bmo)
Option 1 wfm.
Flags: needinfo?(dao+bmo)
No longer depends on: 1375012
Comment on attachment 8982469 [details]
Bug 1457719 - Part 2 - Disable the reftest for "arrowscrollbox" scrolling for timing issues. rs=dao

rs=dao from comment 27.
Attachment #8982469 - Flags: review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b0ef8c7bf8
Part 1 - Restore "scrollbox.css" as a XBL stylesheet. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/f352b2883f00
Part 2 - Disable the reftest for "arrowscrollbox" scrolling for timing issues. rs=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b014d30c222
Part 3 - Replace "autorepeatbutton" with "toolbarbutton" in the base "arrowscrollbox" binding. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dc8b91fc04
Part 4 - Remove the "autorepeatbutton" element. r=bz
Depends on: 1466467
Depends on: 1466763
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: