Page MenuHomePhabricator

Entire page preview should be clickable
Open, LowPublic

Description

Update [08/08/18]
The original fix for this patch caused another regression (T200940) - by making it impossible to ctrl+command+click links into new tabs. The patch was reverted, this task reopened with updated acceptance criteria


From T192710: [Feedback] Personal observations about the page previews feature usability

  • I feel the area that is clickable on the panel is strange- I guess it is only on the part that is 100% visible, and that a gap has to be added between the preferences and the button, but maybe the gap can be just a corner on the right-down, or at least extend the clickable parts a bit more towards the bottom part? Image attached:

Problem

For readers it is unclear where the clickable area on the page previews panel begins. Intuitively one might think that the content (image + text) is clickable, but currently the bottom portion of the text content is not clickable.

When fixing this, we'll also need to make sure that we don't break default browser behaviour, for example, opening a link in a new tab via keyboard shortcuts (command+ctrl+click)

Design notes

  • the entire panel, aside from the cog+cog hover area, should click-through to the respective page
  • the entire panel should have pointer:cursor

Developer notes

Currently the links only apply to the thumbnail portion and the extra portion via links inside the preview itself (note the red line surrounding the link inside this preview:)

Proposed solution

We need to propose a solution.

Bind a click to anywhere on the preview to the link via JS. The settings cog should stop propagation to the parent element. Note, there is a danger of accessibility problems here but these are recorded and will be dealt with later. [Update: doesn't work. Breaks open in new tab]

QA

This can be tested on http://reading-web-staging.wmflabs.org/

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 23 2018, 12:23 PM
alexhollender triaged this task as Normal priority.Apr 25 2018, 6:00 PM
alexhollender updated the task description. (Show Details)

@Jdlrobson can you take a look at this when you have a second? I'd love to move it up in the queue if it doesn't seem to complicated

Jdlrobson updated the task description. (Show Details)Apr 25 2018, 6:39 PM

@alexhollender added some thoughts. Seems a bit of risk here. Answer the questions and throw it into needs analysis in the backlog so it enters the developer queue :)

alexhollender updated the task description. (Show Details)May 1 2018, 3:55 PM

@Jdlrobson just put answers to your questions in a Design notes section

Let's attempt to estimate this with the other developers and see what comes back.

It's also a little tricky to select the text since the cursor isn't as an I-beam.

@Niedzielski ah that's a good point. I guess in this case clicking is more common than selecting text. Maybe we can't have our cake and eat it too? Will try to think of a solution here, let me know if you've got any ideas.

@Jdlrobson I think this is good to go? Design notes in the description describe what the UX should be.

@alexhollender it's not clear how this impacts accessibility and which option to go with.
In particular, it's not clear whether the area around the settings cog is clickable or not.
I think what would help is a red polygon detailing the clickable region (similar to developer notes but showing what we'd like to get to). Can that be arranged?

@Jdlrobson let me know if this clarifies. Regarding accessibility: I did speak to Volker about this. No conclusion thus far, but I think we can keep that a separate topic (tracked here T195718).

Jdlrobson renamed this task from Clickable area on the panel is strange to Entire page preview should be clickable.Jun 19 2018, 5:02 PM
Jdlrobson updated the task description. (Show Details)

The footer element is blocking the lower part of the popup. Is there any reason this can't be fixed just by fiddling with the CSS so that its width doesn't stretch across the whole element, and repositioning things so that this doesn't affect the appearance?

Jdlrobson set the point value for this task to 3.Jun 26 2018, 4:23 PM
Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

Change 442880 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Whole popup area should be clickable

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

Change 442880 merged by Jdlrobson:
[mediawiki/extensions/Popups@master] Whole popup area should be clickable

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

Jdlrobson updated the task description. (Show Details)

Tested this quite a bit yesterday. Looks good to me on staging. Tested on Windows and Mac and Chrome, Firefox, Safari, IE and Edge.

ovasileva closed this task as Resolved.Jul 4 2018, 10:49 AM
ovasileva added a subscriber: ovasileva.

Looks good!

Jdlrobson updated the task description. (Show Details)Aug 8 2018, 9:25 PM
Jdlrobson removed the point value for this task.
Jdlrobson reopened this task as Open.Aug 8 2018, 9:38 PM

Patch has been reverted. We'll need to rethink the approach here at a later date.

Jdlrobson updated the task description. (Show Details)Aug 8 2018, 9:38 PM
Jdlrobson removed ABorbaWMF as the assignee of this task.Aug 10 2018, 8:52 PM
Jdlrobson lowered the priority of this task from Normal to Low.
Jdlrobson added a subscriber: ABorbaWMF.

Marking as low for the time being. Correct me if I'm wrong!

Marking as low for the time being. Correct me if I'm wrong!

Sounds good to me.

@Jdlrobson @ovasileva why low priority? Thinking that since page previews are front & center this bug is pretty visible

Patch has been reverted. We'll need to rethink the approach here at a later date.

What ended up happening with this patch?

How would y'all feel about bringing it onto the sprint board? I feel like the priority should be higher than low.

@alexhollender not sure I understand. The original fix here caused T200940 so we reverted it. We don't currently know a way to fix this without a big risky rewrite of the page previews render code.

We'd need to schedule some analysis (spike?) if we want to pursue a solution to this problem.
My preference is to wait till we have less projects in flight to do that.

@alexhollender not sure I understand. The original fix here caused T200940 so we reverted it. We don't currently know a way to fix this without a big risky rewrite of the page previews render code.
We'd need to schedule some analysis (spike?) if we want to pursue a solution to this problem.
My preference is to wait till we have less projects in flight to do that.

+ 1 on this. We'd want to fix this eventually but as it seems fairly complex and we have higher-priority project in-flight, I'd wait and reconsider a bit further down the line. We can bump up the priority then.