Summary: | Padding in popup menu gets lost with styled <select> in Windows | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||||||||||||||||||
Component: | Forms | Assignee: | Brian Weinstein <bweinstein> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | adele, aroben, darin, eric, sfalken | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | Windows 7 | ||||||||||||||||||||||
Attachments: |
|
Description
Brian Weinstein
2010-01-05 16:18:06 PST
Created attachment 45938 [details]
[PATCH] Fix
Comment on attachment 45938 [details]
[PATCH] Fix
Where's the test?
Looks sane to me, but I'm probably not the perfect reviewer for this.
Seems we need at least a manual test. Eventually we need to invent a way to layout-test popup windows as they are a large source of manual tests. :(
Comment on attachment 45938 [details]
[PATCH] Fix
D'oh, forgot about the manual test, clearing review flag, will post a new patch with manual test.
Created attachment 45971 [details]
[PATCH] Fix with manual test
Created attachment 45972 [details]
[PATCH] Typo Fix
Comment on attachment 45972 [details]
[PATCH] Typo Fix
This still looks sane to me. I think this probably still should have Adam Roben, or someone who is more familiar with the windows build than I give this a thumbs up.
Comment on attachment 45972 [details]
[PATCH] Typo Fix
There's a comment in PopupMenu::calculatePositionAndSize:
// Add padding to align the popup text with the <select> text
Does your patch preserve that attribute?
(In reply to comment #7) > (From update of attachment 45972 [details]) > There's a comment in PopupMenu::calculatePositionAndSize: > > // Add padding to align the popup text with the <select> text > > Does your patch preserve that attribute? Yes, this only adds additional padding if the width of the text is smaller than the control, which isn't the default select case. The text is still aligned. (In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 45972 [details] [details]) > > There's a comment in PopupMenu::calculatePositionAndSize: > > > > // Add padding to align the popup text with the <select> text > > > > Does your patch preserve that attribute? > > Yes, this only adds additional padding if the width of the text is smaller than > the control, which isn't the default select case. The text is still aligned. It seems like the text will not be aligned in a case like this: <select style="width:100px"> <option>some really long text, longer than 100px</option> </select> (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (From update of attachment 45972 [details] [details] [details]) > > > There's a comment in PopupMenu::calculatePositionAndSize: > > > > > > // Add padding to align the popup text with the <select> text > > > > > > Does your patch preserve that attribute? > > > > Yes, this only adds additional padding if the width of the text is smaller than > > the control, which isn't the default select case. The text is still aligned. > > It seems like the text will not be aligned in a case like this: > > <select style="width:100px"> > <option>some really long text, longer than 100px</option> > </select> (Might need a "padding:0" in that style attribute to get the text out of alignment.) (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > (From update of attachment 45972 [details] [details] [details] [details]) > > > > There's a comment in PopupMenu::calculatePositionAndSize: > > > > > > > > // Add padding to align the popup text with the <select> text > > > > > > > > Does your patch preserve that attribute? > > > > > > Yes, this only adds additional padding if the width of the text is smaller than > > > the control, which isn't the default select case. The text is still aligned. > > > > It seems like the text will not be aligned in a case like this: > > > > <select style="width:100px"> > > <option>some really long text, longer than 100px</option> > > </select> > > (Might need a "padding:0" in that style attribute to get the text out of > alignment.) The text is in alignment in this case as well, I will upload screenshots. Created attachment 45986 [details]
Screenshot of Example (ToT w/ Patch)
Created attachment 45987 [details] Screenshot of Example (r52733 - Before Patch) Created attachment 45989 [details]
Screenshot of Example w/ WebKit Appearance Off (ToT with Patch)
Created attachment 45990 [details] Screenshot of Example w/ WebKit Appearance Off (r52733) Created attachment 46087 [details]
[Patch] Different Fix
Instead of forcing padding left and right, support padding-right when webkit-appearance is off.
Comment on attachment 46087 [details]
[Patch] Different Fix
What effect does this patch have on <input type=search results=10>? What effect does it have on Mac?
Maybe we should only return paddingRight() when our appearance in MenulistButtonPart?
(In reply to comment #17) > (From update of attachment 46087 [details]) > What effect does this patch have on <input type=search results=10>? What effect > does it have on Mac? > > Maybe we should only return paddingRight() when our appearance in > MenulistButtonPart? If webkit-appearance is off, style()->appearance() returns NoControlPart. (In reply to comment #17) > (From update of attachment 46087 [details]) > What effect does this patch have on <input type=search results=10>? What effect > does it have on Mac? > > Maybe we should only return paddingRight() when our appearance in > MenulistButtonPart? And Mac never calls clientPaddingRight. (In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 46087 [details] [details]) > > Maybe we should only return paddingRight() when our appearance in > > MenulistButtonPart? > > If webkit-appearance is off, style()->appearance() returns NoControlPart. I see, I was confused by the comment in the code. Comment on attachment 46087 [details] [Patch] Different Fix > + <script> > + // Never have the selects show the selected index, because it garbles the select. > + window.addEventListener('load', function() { > + var selects = document.querySelectorAll('select'); > + for (i = 0; i < selects.length; i++) { > + selects[i].options.selectedIndex = -1; > + selects[i].addEventListener('change', function() { > + this.options.selectedIndex = -1; > + }); > + } > + }); > + </script> Why is this script needed? > + <div class="multi-button"> > + <p style="display: inline">Select: </p> Why are the div and p elements needed? (In general, I think it's good to have test cases be as minimal and focused as possible.) > int RenderMenuList::clientPaddingRight() const > { > + if (style()->appearance() == MenulistPart) > + // If the appearance is MenulistPart, then the select isn't styled, so > + // we want to return the standard endOfLinePadding. We can't add paddingRight() > + // because that value includes the width of the dropdown button so we must use > + // our own endOfLinePadding constant. > + return endOfLinePadding; You need braces around this if, since it contains more than a single line. I think you need to also check to see if style()->appearance() == MenulistButtonPart. We should return endOfLinePadding in that case, too. You can test this using <select style="border:1px solid black; width:40px">. You should add that to your test case. We also need to make a similar change to clientPaddingLeft(). For RTL <select>s, the button is on the left! I think we'll need to detect what direction the <select> is following (using style->direction()) and act accordingly. (You should add a test for an RTL <select>, too.) I think the comment needs to be reworded a little more from its original form in PopupMenuWin. I'd say something like: // For these appearance values, the theme applies padding to leave room for the drop-down button. // But leaving room for the button inside the popup menu itself looks strange, so we return a // small default padding to avoid having a large empty space appear on the side of the popup menu. r- so we can fix the MenulistButtonPart and RTL issues. (In reply to comment #21) > (From update of attachment 46087 [details]) > > + <script> > > + // Never have the selects show the selected index, because it garbles the select. > > + window.addEventListener('load', function() { > > + var selects = document.querySelectorAll('select'); > > + for (i = 0; i < selects.length; i++) { > > + selects[i].options.selectedIndex = -1; > > + selects[i].addEventListener('change', function() { > > + this.options.selectedIndex = -1; > > + }); > > + } > > + }); > > + </script> > > Why is this script needed? It was so the selected option never shows up in the select, just because the select is too small, not really needed though, I can get rid of it. > > + <div class="multi-button"> > > + <p style="display: inline">Select: </p> > > Why are the div and p elements needed? (In general, I think it's good to have > test cases be as minimal and focused as possible.) This was a reduction from a test case that was given to me, and I was able to do a lot more reducing than I thought. I'll fix this. > > > int RenderMenuList::clientPaddingRight() const > > { > > + if (style()->appearance() == MenulistPart) > > + // If the appearance is MenulistPart, then the select isn't styled, so > > + // we want to return the standard endOfLinePadding. We can't add paddingRight() > > + // because that value includes the width of the dropdown button so we must use > > + // our own endOfLinePadding constant. > > + return endOfLinePadding; > > You need braces around this if, since it contains more than a single line. I forget the rule when some of the lines are comments. I'll fix that. > > I think you need to also check to see if style()->appearance() == > MenulistButtonPart. We should return endOfLinePadding in that case, too. You > can test this using <select style="border:1px solid black; width:40px">. You > should add that to your test case. I'll add another select with these properties. > We also need to make a similar change to clientPaddingLeft(). For RTL > <select>s, the button is on the left! I think we'll need to detect what > direction the <select> is following (using style->direction()) and act > accordingly. (You should add a test for an RTL <select>, too.) > RTL in popups was turned off in http://trac.webkit.org/changeset/23372 to match Mac, so I don't think this needs to happen. > I think the comment needs to be reworded a little more from its original form > in PopupMenuWin. I'd say something like: > > // For these appearance values, the theme applies padding to leave room for the > drop-down button. > // But leaving room for the button inside the popup menu itself looks strange, > so we return a > // small default padding to avoid having a large empty space appear on the side > of the popup menu. I will clean up the comment. > > r- so we can fix the MenulistButtonPart and RTL issues. I'll upload a new patch with a cleaned up test and MenulistButtonPart fixed. Thanks for the review. Created attachment 46092 [details]
[PATCH] Fix with Adam's Comments
Comment on attachment 46092 [details]
[PATCH] Fix with Adam's Comments
This looks good.
Landed in r52958. |