Page MenuHomePhabricator

Search input in new heading not cursor-discoverable ("readonly")
Closed, ResolvedPublic3 Estimate Story Points

Description

When hovering over the search field on tablet/desktop (before clicking on it), the cursor does not in any way reveal that there is a clickable area.

At first I thought the mouse events were hijacked due to the presence of div.transparent-shield.cloaked-element (which would be addressed by setting pointer-events: none; so that hover and focus events will go through to the underlying elements instead).

However, that was a red herring. It seems the real culprit is the presence of a readonly attribute on the input field. Why is this there?

Plan (YMMV)

  • Check if the original bug affects Grade A browsers.
    • If it doesn't, remove the readonly attribute and take it easy for the rest of the day.
    • If it does, then:
  • T161763#3162791?

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterShow cursor:text when pointing over search input

Event Timeline

Krinkle created this task.Mar 29 2017, 9:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 29 2017, 9:44 PM
Krinkle updated the task description. (Show Details)Mar 29 2017, 9:44 PM
ovasileva triaged this task as High priority.Apr 4 2017, 3:52 PM
Jdlrobson renamed this task from [Regression] Search input in new heading not cursor-discoverable ("readonly") to Search input in new heading not cursor-discoverable ("readonly").Apr 6 2017, 5:19 PM
Jdlrobson removed a project: Regression.
Jdlrobson added a subscriber: Jdlrobson.EditedApr 6 2017, 5:21 PM

This is not actually a regression. The old header had the same issue.
The field is actually readonly for non-js users as the user doesn't interact with it - we pop open a search overlay when clicked.

Looking into the code the reason for this is explained in the comment:

		// don't use focus event (https://bugzilla.wikimedia.org/show_bug.cgi?id=47499)
		//
		// focus() (see SearchOverlay#show) opens virtual keyboard only if triggered
		// from user context event, so using it in route callback won't work
		// http://stackoverflow.com/questions/6837543/show-virtual-keyboard-on-mobile-phones-in-javascri

Seems like we have a couple of options

  1. See if this is needed any more. We might be able to switch to use a focus event (rEMFR83ef66aa660c: Don't enable search enhancement on Android 2 suggests it was added for Android 2 which is now a grade C browser)
  2. Add cursor: text; to the readonly field.

@phuedx any other ideas?

phuedx added a comment.EditedApr 7 2017, 9:14 AM

@phuedx any other ideas?

First, as you've said, double check whether any Grade A mobile UA's exhibit the same behavior as the Android 2 browser.

Add another element to the DOM immediately after the search box definition that looks like <input type="search" class="search" ... >, has tabindex="0" – is focusable with a keyboard, which readonly="" elements are but UA's will style them differently when they are focussed – and when tapped opens the search overlay, i.e.

<input class="search no-js-only" type="search" name="search" autocomplete="off" placeholder="Search Wikipedia" value="">
<div class="search js-only" tabindex="0">Search Wikipedia</div>

This coupled with your cursor: text; suggestion should work for all UA's regardless of any virtual keyboard idiosyncrasies.

@Jdlrobson @phuedx I assume the readonly is for js clients not no-js clients. If you apply readonly to no-js clients they can't enter any text.

phuedx added a comment.Apr 8 2017, 4:56 AM

@Jdlrobson @phuedx I assume the readonly is for js clients not no-js clients. If you apply readonly to no-js clients they can't enter any text.

Hah! Yes. See T161763#3161013. I've updated my comment accordingly.

phuedx updated the task description. (Show Details)Apr 10 2017, 5:20 PM

This seems actionable to me... isn't the analysis part actually doing the task now @phuedx ? I'd suggest a timeboxed 2 pointer to avoid scope creep.

phuedx added a subscriber: ovasileva.

@Jdlrobson: Yes. I've moved to to Triaged but Future accordingly /cc @ovasileva

Jdlrobson lowered the priority of this task from High to Medium.Apr 13 2017, 6:58 PM
Jdlrobson moved this task from Upcoming to Triaged but Future on the Readers-Web-Backlog board.

This task has been neglected for over a month now. We might want to consider it for an upcoming sprint.

Jdlrobson updated the task description. (Show Details)May 30 2017, 4:44 PM
ovasileva set the point value for this task to 3.May 30 2017, 4:45 PM

(For the record, I just verified that current production still adds readonly with JavaScript after page load, and still causes the discoverability/accessibility issue.)

Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Jul 13 2017, 6:04 PM
pmiazga claimed this task.Jul 21 2017, 2:23 PM

In short - <input> was not discoverable by:

  • safari
  • opera
  • some IE versions
  • Browser on android.

In some browsers "cursor:text" was applied only when the mouse pointer was over input placeholder.

I went with Sam idea and introduced a new element which is visible only for JS users. Tested on:

  • IE10 @ Windows7
  • IE11 @Windows 8, 10
  • Edge @ Windows 10,
  • Firefox (latest) @ Windows 7, OSX, Linux
  • Opera (latest) @ Windows 7, OSX
  • Chrome 59 @ Windows 7, OSX, Linux
  • Browser @ Note 10.1/Android 5.1

Works properly now.

Change 368816 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Show cursor:text for JS-only users when hovering over search input

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

Change 368816 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Show cursor:text when pointing over search input

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

per @bmansurov suggestion, I changed the solution to add only cursor:text style and it looks like it solves the problem.

Change 368816 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Show cursor:text when pointing over search input

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

phuedx removed pmiazga as the assignee of this task.Aug 1 2017, 7:36 AM
phuedx added a subscriber: pmiazga.
phuedx added a comment.Aug 1 2017, 8:23 AM

Any reason this skipped Needs QA?

This can skip QA as I believe it would be better verified by a developer who understands concretely what "grade A" browser means and requires some understanding of how browser accessibility works (how to use screen reader). I would like a developer to test and sign this off that's not @pmiazga

(posted draft - failed to hit submit)

Niedzielski updated the task description. (Show Details)Aug 2 2017, 12:31 PM

I was involved during review, I'd like to not sign off.

I'd appreciate if somebody would update the task with proper AC for signoff and maybe if you feel generous even QA steps.

The task is long and with a lot of comments and after reading it all I'm unsure of exactly how or what to sign off with how it is specified right now.

pmiazga updated the task description. (Show Details)Aug 2 2017, 5:03 PM

Apparently from discussion:

  • This is a minerva desktop bug
  • Cursor when hovering search input should be the text caret
  • List of browsers to test on to be determined (See previous comment)

This is a minerva desktop bug

There is no mouse cursor on touch devices. This can be replicated on the mobile site or any old school browser that has or uses a cursor :)

It can be also done on touch devices with a pointing device like Samsung Note 10.1 (it has an S-Pen, if you keep S-Pen couple millimeters above the screen it will act as a mouse pointer).

So, this is what I wanted to read... It takes little time and makes a world of difference to people doing signoff or QA:

QA steps

  • Load the URL on a window with tablet or desktop width (search input visible)
  • Hover over the text input with the mouse
  • Expected: Cursor changes to the text caret
    • Actual: Cursor stays with it's default form

Could reproduce on https://en.m.wikipedia.org/wiki/Dog with

  • Chrome 59
  • Safari 9.1.1

In don't see why we skipped QA on this one really, if we specify this ^ I think our QA would have been easily able to test it and more so in a bunch of browsers.

Jhernandez closed this task as Resolved.Aug 3 2017, 9:40 AM
Jhernandez claimed this task.

Signing off:

Tested fix on https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog with

  • Chrome 59
  • Safari 9.1.1
  • Firefox 54

Seems good. If the steps are not right or we need to test in other specific browsers please state so so that we can move this to QA then.