Page MenuHomePhabricator

Mobile web search x clear button masked at its bottom
Closed, ResolvedPublic2 Story Points

Description

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.

Event Timeline

dr0ptp4kt created this task.Aug 8 2016, 3:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 8 2016, 3:20 PM
Jdlrobson triaged this task as Normal priority.Aug 8 2016, 5:14 PM

What browser and device?

This particular screenshot was from Safari on iOS 9.3 on a phone.

Change 303708 had a related patch set uploaded (by Bmansurov):
Vertically align the icon pseudo elements

https://gerrit.wikimedia.org/r/303708

bmansurov claimed this task.Aug 9 2016, 5:08 PM

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!

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.

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.

Nirzar added a comment.Aug 9 2016, 9:49 PM

Change 303925 had a related patch set uploaded (by Bmansurov):
Use SVG version of the clear icon

https://gerrit.wikimedia.org/r/303925

Change 303925 merged by jenkins-bot:
Use SVG version of the clear icon

https://gerrit.wikimedia.org/r/303925

@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.

  • Android 2.3 Browser phone form factor
  • Android 4.x or higher Chrome phone form factor
  • Android 4.x or higher Chrome tablet form factor
  • iOS 9.3 iPhone Safari
  • iOS 9.3 iPad Safari
  • Opera Mini non-compressed (not extreme mode) Android 4.x or higher phone form factor
  • UC browser (not mini with compression) Android 4.x or higher phone form factor
  • Windows 7.5+ Phone IE
  • Desktop Firefox
  • Desktop Chrome
  • Desktop Safari
  • Internet Explorer 11
  • Edge
MBinder_WMF set the point value for this task to 2.Aug 10 2016, 5:14 PM

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:

This is the SVG

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...) ?

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

https://gerrit.wikimedia.org/r/304144

Change 304144 abandoned by Jdlrobson:
Use a tag for clear icon

Reason:
Styling ::after sounds nicer to me if that is indeed the issue.

https://gerrit.wikimedia.org/r/304144

Change 304296 had a related patch set uploaded (by Jdlrobson):
MWUI: Vertically align the icon pseudo elements

https://gerrit.wikimedia.org/r/304296

bmansurov reassigned this task from bmansurov to Jdlrobson.Aug 12 2016, 9:54 AM

Assigning to @Jdlrobson as he submitted a follow up patch.

@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.

Change 303708 merged by jenkins-bot:
MWUI: Vertically align the icon pseudo elements

https://gerrit.wikimedia.org/r/303708

To test please visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page#/search and type something in the search box.

looks good! i will create a follow up for sizing of the icon.

Change 304296 abandoned by Bartosz Dziewoński:
MWUI: Vertically align the icon pseudo elements

Reason:
Superseded by https://gerrit.wikimedia.org/r/#/c/303708/

https://gerrit.wikimedia.org/r/304296

phuedx added a comment.EditedAug 18 2016, 6:14 PM
  • Android 2.3 Browser phone form factor – clear button not shown on an emulated Galaxy S2
  • Android 4.x or higher Chrome phone form factor – 49.0.2623.91 on a Galaxy S7
  • Android 4.x or higher Chrome tablet form factor – 49.0.2623.91 on a Galaxy Tab 4
  • iOS 9.3 iPhone Safari
  • iOS 9.3 iPad Safari
  • Opera Mini non-compressed (not extreme mode) Android 4.x or higher phone form factor – clear button not shown
  • UC browser (not mini with compression) Android 4.x or higher phone form factor – 11.0.0.828 on a Nexus 9
  • Windows 7.5+ Phone IE – Windows Phone 8.1 on an emulated Lumia 520
  • Desktop Firefox – 47.0 on OS X El Capitan (10.11.6)
  • Desktop Chrome – 52.0.2743.116 on OS X El Capitan (10.11.6)
  • Desktop Safari – 9.1.2 on OS X El Capitan (10.11.6)
  • Internet Explorer 11 – IE11 on Windows 7
  • Edge – 14 on Windows 10
phuedx closed this task as Resolved.Aug 19 2016, 11:56 AM

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:

@phuedx can you please create a new ticket ^?

That is not a mw-ui-icon so likely to be unrelated

That is not a mw-ui-icon so likely to be unrelated

If I did, then I didn't mean to imply that this was the case. I'll be creating a new task shortly…