Use AppConstants in tabbrowser.xml

RESOLVED FIXED in Firefox 39

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

34 Branch
Firefox 39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

13.34 KB, patch
Gavin
: review+
Neil Deakin (not available until Aug 9)
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
Created attachment 8577307 [details] [diff] [review]
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.
Attachment #8577307 - Flags: review?(gavin.sharp)
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? :(
Attachment #8577307 - Flags: review?(gavin.sharp)
Attachment #8577307 - Flags: review+
Attachment #8577307 - Flags: feedback?(enndeakin)
(Assignee)

Comment 2

2 years ago
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 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.
Attachment #8577307 - Flags: feedback?(enndeakin) → feedback+
(Assignee)

Updated

2 years ago
Blocks: 1145734
(Assignee)

Comment 4

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

I filed bug 145734 as a follow-up to address comment 3.
https://hg.mozilla.org/mozilla-central/rev/347dbe653fb3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1146193

Updated

4 months ago
Depends on: 1349502
You need to log in before you can comment on or make changes to this bug.