Clean up appPicker's use of XUL

RESOLVED FIXED

Status

()

Toolkit
Preferences
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
Bug 498153 prompted me to look at the XUL underlying the app picker.
(Assignee)

Comment 1

9 years ago
Created attachment 385715 [details] [diff] [review]
Proposed patch

I wondered why it went to all the trouble of creating custom XUL that is already readily available via the listbox element.

I switched from overloading the value property to using a handlerApp property (this hack only works because the XBL gets bound later).

I stopped setting identical ids on multiple elements. In fact I don't set any ids at all! I did change the listbox's ID to avoid breaking bug 498153.

I removed an unnecessary vbox, and other minor tweaks.

I also moved the placement of the Browse button, but I can readily revert that.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #385715 - Flags: review?(gavin.sharp)

Comment 2

9 years ago
(In reply to comment #1)
> Created an attachment (id=385715) [details]
> Proposed patch
> 
> I wondered why it went to all the trouble of creating custom XUL that is
> already readily available via the listbox element.
> 
> I switched from overloading the value property to using a handlerApp property
> (this hack only works because the XBL gets bound later).
> 
> I stopped setting identical ids on multiple elements. In fact I don't set any
> ids at all! I did change the listbox's ID to avoid breaking bug 498153.
> 
> I removed an unnecessary vbox, and other minor tweaks.
> 
> I also moved the placement of the Browse button, but I can readily revert that.

Probably because this was the very first xul dialog I ever authored. :) Although it did make it through reviews. :O
(Assignee)

Comment 4

9 years ago
Pushed changeset acd5dee9dab3 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

8 years ago
Comment on attachment 385715 [details] [diff] [review]
Proposed patch

>+.listcell-label {
>+  margin: 5px;
Unfortunately a listcell-label has a margin: 0px !important (and some padding) so this rule has no effect. Which would you prefer,
a) Just remove the margin rule
b) Replace the margin with 5px padding
c) Something else?
(Assignee)

Comment 6

8 years ago
Created attachment 421065 [details] [diff] [review]
Spacing fix

Patch following b) above, since that was Gavin's "obvious choice".

I also swapped the width and height into toolkit's normal order.
Attachment #421065 - Flags: review?(dao)

Updated

8 years ago
Attachment #421065 - Flags: review?(dao) → review+
(Assignee)

Comment 7

8 years ago
Comment on attachment 421065 [details] [diff] [review]
Spacing fix

Pushed changeset cea14ad20b61 to mozilla-central.

Comment 8

8 years ago
Could this have caused the file (view, bookmarks and help exhibit this new behavior) menus to shift left and right when navigating between file menu with sub-folders?  I'm seeing a one/two pixel shift left and right using: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre ID:20100111171142

~B
(Assignee)

Comment 9

8 years ago
These patches only ever affected one dialog used in certain places on Windows.
You need to log in before you can comment on or make changes to this bug.