The default bug view has changed. See this FAQ.

Arrow of drop-down list should not be affected by padding

RESOLVED FIXED in mozilla30

Status

()

Core
Layout: Form Controls
P4
minor
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sebo, Assigned: Alex Henrie)

Tracking

({dev-doc-complete, site-compat, testcase})

Trunk
mozilla30
dev-doc-complete, site-compat, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8365593 [details]
dropDownListArrow.html

The display of the arrow inside a drop-down list should not be affected by the padding set for the <select> element.

This was first mentioned in comment 144 of bug 157846.

Sebastian
(Reporter)

Comment 1

3 years ago
Created attachment 8365594 [details]
dropDownListArrow.png

Expected display of the drop down arrow in Windows 8.1
(Assignee)

Comment 2

3 years ago
Created attachment 8365646 [details] [diff] [review]
Proposed patch

I agree that this is a problem. Putting the combobox button inside the padding is ugly and doesn't match the native themes or Chrome's rendering.

I've written a patch that resolves the issue everywhere but B2G: https://tbpl.mozilla.org/?tree=Try&rev=bc6beacc444d

If Robert lends a hand again, I think we can get this resolved pretty quick.
Attachment #8365646 - Flags: feedback?(roc)
(Assignee)

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
The proposed fix breaks this test:
data:text/html,<select%20style="padding-top:10%;border:solid"><option>XXXXXXXXXXXXXXXXXXXXXX</select>
Maybe the problem is the switch from Used values to Computed values?

Please add a <select style=direction:rtl> to the test.

Updated

3 years ago
Severity: normal → minor
Keywords: testcase
OS: Windows 8.1 → All
Priority: -- → P4
Hardware: x86_64 → All
The patch breaks <select style="border:solid;"> too.
(I'm testing this on Linux, in case it matters.)
(Assignee)

Comment 5

3 years ago
Created attachment 8384874 [details] [diff] [review]
Proposed patch (try 2)

Well, I'm a bit closer. This patch includes an RTL test and works fine with the two examples you gave. Thanks for the suggestion about computed vs. used values.

Unfortunately, this patch causes a problem with layout/reftests/bugs/557087-2.html on Windows: https://tbpl.mozilla.org/?tree=Try&rev=0418cb20316f

I've tried everything I could think of with no success. Any more suggestions?
Attachment #8365646 - Attachment is obsolete: true
Attachment #8365646 - Flags: feedback?(roc)
Attachment #8384874 - Flags: feedback?(roc)
Attachment #8384874 - Attachment is patch: true
Try adding some dumping code to nsComboboxControlFrame::Reflow to dump the position of the button, and push that to try.
(Assignee)

Comment 7

3 years ago
Created attachment 8385461 [details] [diff] [review]
Proposed patch (try 3)

After some more trial and error, I've found a combination that only fails on Android and B2G: https://tbpl.mozilla.org/?tree=Try&rev=302eec78265b

I'm thinking I should try setting -moz-appearance:menulist on the reference divs and see if that resolves the problem. But because I'm not set up to test Android or B2G, it's hard for me to tell.
Attachment #8384874 - Attachment is obsolete: true
Attachment #8384874 - Flags: feedback?(roc)
Attachment #8385461 - Flags: feedback?(roc)
(Assignee)

Comment 8

3 years ago
Never mind, adding -moz-appearance:menulist to the reference would defeat the purpose of the test.
That failure is just because mobile/android/themes/core/content.css has:
/* Override inverse OS themes */
select,
textarea,
button,
xul|button,
* > input:not([type="image"]) {
  -moz-appearance: none !important;  /* See bug 598421 for fixing the platform */
  border-radius: 3px;
}
and your reference DIVs don't.

You can work around this by adding border-radius:0 to the selects in padding-button-placement.html.
(Assignee)

Comment 10

3 years ago
Created attachment 8386596 [details] [diff] [review]
Proposed patch (try 4)

Try 4: https://tbpl.mozilla.org/?tree=Try&rev=b3f5a71b3a58

Android is happy now (thanks for the tip!), but B2G ICS isn't. From the try server's screenshots, it looks like it gets caught in the middle of rendering.

I tried adding needs-focus to the test and it still failed on B2G ICS: https://tbpl.mozilla.org/?tree=Try&rev=e389bd413f00

I tried adding a 1-second delay to the test and it still failed on B2G ICS: https://tbpl.mozilla.org/?tree=Try&rev=c301e965550d

Fixing this bug also fixes bug 893509, because the NS_ASSERTION is removed entirely.

For the record, the fuzzy(1,4) is for Mac OS X: https://tbpl.mozilla.org/?tree=Try&rev=f54627d918ae

Robert, you've been really helpful...do you have any more insights here?
Attachment #8385461 - Attachment is obsolete: true
Attachment #8385461 - Flags: feedback?(roc)
Attachment #8386596 - Flags: feedback?(roc)
(In reply to Alex Henrie from comment #10)
> Try 4: https://tbpl.mozilla.org/?tree=Try&rev=b3f5a71b3a58
> 
> Android is happy now (thanks for the tip!), but B2G ICS isn't. From the try
> server's screenshots, it looks like it gets caught in the middle of
> rendering.
> I tried adding needs-focus to the test and it still failed on B2G ICS:
> https://tbpl.mozilla.org/?tree=Try&rev=e389bd413f00

Those try-pushes don't have the border-radius:0 fix.

They show two failures but the second one is bogus. I know it looks like we only rendered half the page but I think that's just logging stuff going crazy and being misinterpreted by the reftest analyzer. So focus on the first failure.

Other than the border-radius issue, the problem seems to be that the black borders aren't properly covering the drop-down button. Try making the covering abs-pos DIVs z-index:1? Here's what content.css has:

select > button {
  border-width: 0px !important;
  margin: 0px !important;
  padding: 0px !important;
  border-radius: 0;
  color: #414141;

  background-image: radial-gradient(at bottom left, #bbbbbb 40%, #f5f5f5), url(arrow.svg) !important;
  background-color: transparent;
  background-position: -15px center, 4px center !important;
  background-repeat: no-repeat, no-repeat !important;
  background-size: 100% 90%, auto auto;

  -moz-binding: none !important;
  position: relative !important;
  font-size: inherit;
}

So it makes sense that the button would be higher in z-order than the abs-pos DIV in your testcase. Which is a bug, really, but not your problem. so add the z-index:1.
(Assignee)

Comment 12

3 years ago
Created attachment 8387161 [details] [diff] [review]
Proposed patch (try 5)

Try 5: https://tbpl.mozilla.org/?tree=Try&rev=847dddda4f45

Sorry, I did have border-radius:0 in https://tbpl.mozilla.org/?tree=Try&rev=f54627d918ae but then I accidentally reverted it. At any rate, it's working now, and your z-index fix worked too. Can you see any other problems?
Assignee: nobody → alexhenrie24
Attachment #8386596 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8386596 - Flags: feedback?(roc)
Attachment #8387161 - Flags: review?(roc)
Attachment #8387161 - Flags: review?(roc) → review+
(Assignee)

Comment 13

3 years ago
Requesting checkin of attachment 8387161 [details] [diff] [review]
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbcdc591ccd
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cfbcdc591ccd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Reporter)

Comment 16

3 years ago
Works fine for me testing on Win 8.1 and Ubuntu 12.04.

Sebastian
Blocks: 893509

Updated

3 years ago
Depends on: 981849

Updated

3 years ago
Depends on: 990655

Updated

3 years ago
Depends on: 1007727
https://developer.mozilla.org/en-US/Firefox/Releases/30/Site_Compatibility#CSS
Keywords: dev-doc-complete, site-compat
Depends on: 1017864

Updated

3 years ago
No longer depends on: 1017864
(Assignee)

Updated

2 years ago
See Also: → bug 157846
You need to log in before you can comment on or make changes to this bug.