| dr0ptp4kt | |
| Aug 8 2016, 3:20 PM |
| F4376563: bs_android_Mobile_Samsung Galaxy Note 2-4.1-720x1280.jpg | |
| Aug 19 2016, 11:56 AM |
| F4376564: bs_android_Mobile_Samsung Galaxy S2-2.3-480x800.jpg | |
| Aug 19 2016, 11:56 AM |
| F4351587: pasted_file | |
| Aug 10 2016, 8:16 PM |
| F4351580: Screen Shot 2016-08-10 at 1.12.00 PM.png | |
| Aug 10 2016, 8:14 PM |
| F4349325: Screen Shot 2016-08-09 at 20.53.07.png | |
| Aug 10 2016, 12:54 AM |
| F4348851: clear-field.svg | |
| Aug 9 2016, 9:49 PM |
| F4345324: x_clear_masked.jpg | |
| Aug 8 2016, 3:20 PM |
As a user accustomed to x clear buttons in other software, when I enter the mobile web language switcher modal dialog box and enter a search character, I expect the x clear button to be a perfect circle.
Presently, the x clear button appears to be masked at its bottom.
Change 303708 had a related patch set uploaded (by Bmansurov):
Vertically align the icon pseudo elements
I say it's a 2 pointer. I spent quite some time trying to find the root cause of the issue.
Looking closely at the code, I can't help but think this will be solved by taking this opportunity to use a SVG with the correct vertical alignment instead of a png for the icon. I don't think the core change should be necessary.
@Nirzar, could you please upload the SVG version of the clear icon? Thanks!
For this specific case, maybe. There maybe other places where we use PNG's rather than SVG's, so the core change would fix all of those.
We shouldn't be using pngs. mw-ui-icon is meant to support SVGs with PNG fallbacks. If a PNG is used that is a bug.
I agree. A quick search in the MF repository resulted in 21 PNG files. Until we find SVG replacements for those, we need to make them work somehow. Also, we'll need that core patch for browsers that don't support SVGs.
Change 303925 had a related patch set uploaded (by Bmansurov):
Use SVG version of the clear icon
@Jdlrobson, the patch has been merged and you can see it here https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page#/search. It doesn't fix the problem though. Here is a screenshot:
Applying the core patch fixes the problem though.
When testing, please check on as many of the following as possible, and feel free to resolve once you're comfortable. I believe an engineer (@Jdlrobson @phuedx @jhobs) should be able to check on multiple devices and @Nirzar you'd want to also do some spot checks. I won't be available for signoff on this task by end of sprint and am booked fully today. I did check some this morning and noticed stuff is still masked, which made sense given the above dialogue.
can we just specify background size for this button?
There are anyways specific styles for this button in there
.search-overlay .clear
.mw-ui-icon-clear.mw-ui-icon-element:before {
background-size:90%;
}
OR---
.search-overlay .clear:before {
background-size:90%;
}also while at it, fix the vertical alignment of the button too
.search-overlay .clear {
top: 14px;
right: 10px;
}No. We should resort to background-size hacks.
The issue with this icon seems to be that the actual SVG is not vertically centered so we should edit the actual SVG file to fix that.
Look:
No. We should resort to background-size hacks.
The issue with this icon seems to be that the actual SVG is not vertically centered so we should edit the actual SVG file to fix that.Look:
That's the problem we're trying to solve. The SVG is the background of ::before, and ::before is not vertically centered.
doesn't .mw-ui-icon.mw-ui-icon-element:before { top: 0; } fix that (I forget why we don't do that in core but there was a reason but we can do that here...) ?
Because it causes T85778. We don't want to do here because it's implementation detail.
I cannot figure out the root cause of why there is a gap between the top of the icon and the pseudo element? Their heights are the same.
... but you can restrict this to simply this case no? e.g. .clear :before { top: 0; } It seems the easiest thing to do.
I expect this is because position: absolute resets the relative position for children.
You'll notice it doesn't happen when .clear is position relative.
Change 304144 had a related patch set uploaded (by Jdlrobson):
Use a tag for clear icon
Change 304144 abandoned by Jdlrobson:
Use a tag for clear icon
Reason:
Styling ::after sounds nicer to me if that is indeed the issue.
Change 304296 had a related patch set uploaded (by Jdlrobson):
MWUI: Vertically align the icon pseudo elements
@bmansurov and I talked about this and we agreed that the issue is that the button tag and mw-ui-icon is not compatible. Ideally we would like to identify why and fix this, but we agreed we should not spend too much time on it.
Timeboxed to 40 minutes I am going to see if I can identify the problem, if not we will simply change the clear icon to an <a> element to address this problem and create a new task to capture the generic problem with <button> elements.
@bmansurov I see we have a line height of 1.4 on the body tag. The default font is 16px so this results in a line height of 22.4px which is bound to result in some rounding issues.
After some investigation I'm inclined to agree with your comment on specifying a line-height value
https://gerrit.wikimedia.org/r/#/c/303708/3/resources/src/mediawiki.ui/components/icons.less
1.5 or 1.5em should suffice.
What say you?
That sounds great. Let's use 1.5em as we want the icon children (pseudo elements) to inherit the computed value rather than to compute their heights themselves.
To test please visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page#/search and type something in the search box.
Change 304296 abandoned by Bartosz Dziewoński:
MWUI: Vertically align the icon pseudo elements
Reason:
Superseded by https://gerrit.wikimedia.org/r/#/c/303708/
Per the above.
During testing, I found that the search icon was improperly placed in Android Browser <= 4.1:
4.1 on a Samsung Galaxy Note:
2.3 on a Samsung Galaxy S2:
If I did, then I didn't mean to imply that this was the case. I'll be creating a new task shortly…