1143038 - Use AppConstants in tabbrowser.xml

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

If you're wondering why it's okay to remove clicktoscroll="true" on the arrowscrollbox, it's because it serves no purpose. The arrowscrollbox has a class="tabbrowser-arrowscrollbox" attribute, and that attribute causes it to use the tabbrowser-arrowscrollbox XBL binding, which inherits from scrollbox.xml#arrowscrollbox-clicktoscroll unconditionally. This change appears to have happened in 2007, so we should probably just call it the expected behavior at this point.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8577307 [details] [diff] [review]
patch

Neil, can you r+ the clicktoscroll change specifically?

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+              if (this.AppConstants.platform == "macosx") {
>+                return dt.effectAllowed = event.altKey ? "copy" : "move";
>+              } else {
>+                return dt.effectAllowed = event.ctrlKey ? "copy" : "move";
>+              }

I think this might be clearer as:

let copyModifier = this.AppConstants.platform == "macosx" ? event.altKey : event.ctrlKey;
return dt.effectAllowed = copyModifier ? "copy" : "move";

>+        if (this.AppConstants.platform == "macosx") {
>+          if (!event.metaKey)
>+            return;
>+        } else {
>+          if (!event.ctrlKey || event.metaKey)
>+            return;
>+        }

Similarly:
let wrongModifiers;
if (this.AppConstants.platform == "macosx") {
  wrongModifiers = !event.metaKey;
} else {
  wrongModifiers = !event.ctrlKey || event.metaKey;
}

if (wrongModifiers)
  return;

>+          let props = { screenX: left, screenY: top };
>+          if (this.AppConstants.platform != "win") {
>+            props.outerWidth = winWidth;
>+            props.outerHeight = winHeight;
>+          }
>+          this.tabbrowser.replaceTabWithWindow(draggedTab, prop);

typo here: prop -> props. No test coverage for this I guess? :(

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Could you please get to this soon Neil? Every time I have to lookup a line number in preprocessed JS or run make in browser/base, I feel a twinge of pain.

Comment 3

[Neil Deakin]

Comment on attachment 8577307 [details] [diff] [review]
patch

This is ok, but I would have removed the special tabbrowser-arrowscrollbox -moz-binding in tabbrowser.css instead so that we consistently use clicktoscroll to get that binding.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/347dbe653fb3

I filed bug 145734 as a follow-up to address comment 3.

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/347dbe653fb3