345710 - scrollbar arrow code abuses look and feel system

Status: NEW

(Core :: XUL, defect)

Description

[[e-mail]]

The look and feel system is hardcoded to return fixed values for the eMetric_ScrollButtonXXXMouseButtonAction values; on GTK2 they are always 0, 1 and 2 while on other platforms they are always 0, 3 and 3. Either the hardcoded values should be moved to nsScrollbarButtonFrame.cpp or the metrics should be fixed to use the look and feel system correctly.

Comment 1

[[e-mail]]

Attachment: hardcode values

Not shown are the backouts of the changes to look and feel.

Comment 2

[[e-mail]]

Attachment: correct usage

Comment 3

[Adam Guthrie]

I really can't review these changes. FWIW, roc was the one who advised me to make these changes in order to fix the win32 bustage.

As roc mentioned, we really don't want to allow people to change these prefs. so I think that using the ifdefs for GTK2 would be OK here.

Comment 4

[[e-mail]]

As I don't have GTK2 could you at least test that the patch you prefer works?

Comment 5

[[e-mail]]

Attachment: hardcode values

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know why you feel that hardcoding execution paths with #ifdefs is preferable to using nsILookAndFeel. I think it's much more ugly and less extensible. The difference in complexity is marginal.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In particular, with the "hardcoded values" patch, if we ever want to support any other behaviour for any button on any other platform, the code will become a complete mess.

Comment 8

[[e-mail]]

(In reply to comment #7)
>In particular, with the "hardcoded values" patch, if we ever want to support
>any other behaviour for any button on any other platform, the code will become
>a complete mess.
Well I guess I could write it slightly differently:
#ifdef MOZ_WIDGET_GTK2
#define LEFTBUTTONACTION pressedButtonAction = 0
#define MIDDLEBUTTONACTION pressedButtonAction = 1
#define RIGHTBUTTONACTION pressedButtonAction = 2
#else
#define LEFTBUTTONACTION pressedButtonAction = 0
#define MIDDLEBUTTONACTION return PR_FALSE
#define RIGHTBUTTONACTION return PR_FALSE
#endif
and leave most of the existing scrollbarbutton code in place.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think that's an improvement, honestly. And I don't know why you find using nsILookAndFeel so upsetting here.

Comment 10

[[e-mail]]

(In reply to comment #9)
>I don't know why you find using nsILookAndFeel so upsetting here.
Because those particular metrics don't work like any of the existing metrics.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsILookAndFeel is an interface. The new settings fit nicely into it. The fact that they're implemented differently shouldn't matter, in my opinion.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.