Page MenuHomePhabricator

CU 2.0: Sticky highlighting in Preliminary check and Compare tabs
Closed, ResolvedPublic5 Estimated Story Points

Description

Goal

In the table of search results from the CU special page, if a user clicks on a highlighted data field, the other highlight items should stay highlighted as the mouse moves.

Mock

https://prtksxna.github.io/wmf-cu-prototype/compare.html

An unhighlighted row
When a row is highlighted, the cell with the mouseover gets the following buttons
Clicking the pin will highlight the button
Notes
  • We wont be putting the funnel button right now
  • There might be some changes to just the buttons later (size, icon etc)
  • Once the user moves their mouse from the cell the buttons go away, but the highlight remains
  • Hovering on the cell again brings back the button, and that is how you unpin it
  • For the above example, all Chrome 65 cells would have had the pin highlighted once it was selected. This way the user can unpin from any highlighted row and doesn't have to find the original row/cell they highlighted from
  • All other highlighting stops if a highlight has been pinned (we might want to change this in the future?)
Acceptance criteria

Things that highlighting works for:

  1. Preliminary check tab
    • Wiki
    • Date attached (Exact matches)
  2. Compare tab
    • IP
    • User-agent

Related Objects

Event Timeline

aezell created this task.Nov 4 2019, 6:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 4 2019, 6:36 PM
Niharika triaged this task as Medium priority.Nov 4 2019, 10:21 PM
Niharika removed a project: Epic.
Niharika added a subscriber: Prtksxna.

This ticket would need a design from @Prtksxna before we can move forward. This is talking about the 'sticky-highlight' that we talked about previously.

Niharika renamed this task from Allow the highlight to be "locked" if a user clicks to CU 2.0: Sticky highlighting in Preliminary check and Compare tabs.Nov 20 2019, 8:09 PM
Niharika assigned this task to Prtksxna.
Niharika updated the task description. (Show Details)

@Prtksxna assigning this to you for the mock and design specs.

Prtksxna updated the task description. (Show Details)Nov 21 2019, 1:21 PM

@Prtksxna We also talked about the sticky highlights showing up in the filters at one point. Are you still planning to add that in?

@Prtksxna We also talked about the sticky highlights showing up in the filters at one point. Are you still planning to add that in?

The way I am thinking about this is — you highlight rows that you're interested in, and filter out rows in which you aren't. By that understanding sticky highlights should never become filters. What do you think?


Per @dbarratt's suggestion we do want to provide a way to efficiently add filters through the data itself. For this I've added a filter icon (maybe a trash icon would be better suited):

Hovering over it will reveal which rows will get removed if this filter is applied:

Finally, clicking it should add the data point to the Filter form and remove the relevant rows.

Should we create a new ticket for this behavior, @Niharika?

@Prtksxna We also talked about the sticky highlights showing up in the filters at one point. Are you still planning to add that in?

The way I am thinking about this is — you highlight rows that you're interested in, and filter out rows in which you aren't. By that understanding sticky highlights should never become filters. What do you think?

Makes sense.


Per @dbarratt's suggestion we do want to provide a way to efficiently add filters through the data itself. For this I've added a filter icon (maybe a trash icon would be better suited):

Hovering over it will reveal which rows will get removed if this filter is applied:

Finally, clicking it should add the data point to the Filter form and remove the relevant rows.

Should we create a new ticket for this behavior, @Niharika?

Yep, that would be great. :)

Niharika updated the task description. (Show Details)Dec 10 2019, 7:46 PM
Niharika set the point value for this task to 5.Dec 10 2019, 8:02 PM
Niharika removed Prtksxna as the assignee of this task.Jan 17 2020, 8:05 PM
Tchanders moved this task from Ready to In Progress on the Anti-Harassment (The Letter Song) board.

Change 573421 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/CheckUser@master] WIP Add highligting on hover to Special:Investigate results tables

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

@Niharika @Prtksxna I have a few questions that arose while working on this:

  • Are the round buttons with a white background a literal design? For now I've kept the OOUI design for the toggleButtonWidget. It seems sensible to have the buttons and colours look consistent with the other OOUI components on the site, so that it's clear these are buttons, and clear when they are toggled on/off.
  • As we've discussed somewhere before, having buttons that appear and disappear means the rows keep changing height (this is also the case on the prototype). Should we have another task to address this and any other design issues that crop up after seeing the working product?
  • Having the pin buttons disappear after a row is pinned makes the UI complicated, because if the check user then wants to unpin that highlight, they may have to look through several rows to find which button they pinned. One solution would be to make all the buttons in the column toggle on and off together, but I think it could be confusing UI to toggle one button on and a different button off. Another solution (which I've implemented) is to keep the pin that was clicked visible when you hover away, but not to show any other pins for the same column. That way, it's clear to the user where they need to click to untoggle the pinned highlight. It also gives a clear indication of how many highlights they have pinned, if they pin several at once. (When many highlights are pinned, it becomes difficult to distinguish between what's highlighted because it was pinned vs because of where you are hovering.) Does this sound OK?

@Niharika @Prtksxna I have a few questions that arose while working on this:

  • Are the round buttons with a white background a literal design? For now I've kept the OOUI design for the toggleButtonWidget. It seems sensible to have the buttons and colours look consistent with the other OOUI components on the site, so that it's clear these are buttons, and clear when they are toggled on/off.

I'll let @Prtksxna respond to that.

  • As we've discussed somewhere before, having buttons that appear and disappear means the rows keep changing height (this is also the case on the prototype).

Should we have another task to address this and any other design issues that crop up after seeing the working product?
I think that's wise. We shouldn't stall on this ticket. There will be a bunch of design issues that we can tackle together in a new ticket.

  • Having the pin buttons disappear after a row is pinned makes the UI complicated, because if the check user then wants to unpin that highlight, they may have to look through several rows to find which button they pinned. One solution would be to make all the buttons in the column toggle on and off together, but I think it could be confusing UI to toggle one button on and a different button off. Another solution (which I've implemented) is to keep the pin that was clicked visible when you hover away, but not to show any other pins for the same column. That way, it's clear to the user where they need to click to untoggle the pinned highlight. It also gives a clear indication of how many highlights they have pinned, if they pin several at once. (When many highlights are pinned, it becomes difficult to distinguish between what's highlighted because it was pinned vs because of where you are hovering.) Does this sound OK?

It sounds okay to me but I would like @Prtksxna's feedback on it too.
@Tchanders This also raises the question about how many highlights do we want to allow at once. When we were discussing this awhile back, I assumed we would only be able to accommodate for one pinned highlight at a time (pin one user-agent and if you pin another, the first one gets unpinned). Am I right in understanding from your comment that it's possible to do multiple highlights at once? In that case, are there any technical reasons to introduce a limit on number of highlights allowed?

Am I right in understanding from your comment that it's possible to do multiple highlights at once? In that case, are there any technical reasons to introduce a limit on number of highlights allowed?

Ah, I don't remember the discussion - I just assumed multiple highlight pinning was required. The current patch lets you pin multiple highlights (but no two cells with the same data). Technically it's fine - we just add classes to all the affected table cells. If that's not what we want design-wise, it can be changed.

I think that's fine. If we can offer more functionality to the user for no extra cost to us, that is great. We can revisit it if we run into problems.
@Tchanders To confirm - sticky highlights persist across paginated tabs right?

@Tchanders To confirm - sticky highlights persist across paginated tabs right?

Is a page refresh acceptable when pinning a row?

If not, I suppose we could create an API that would allow the token to be updated. This would allow the pagination URLs to be updated without a page refresh.

@Tchanders To confirm - sticky highlights persist across paginated tabs right?

@Niharika Not in the current patch. We could do this by adding some extra information to be passed via the token: i.e. the data that should be highlighted when the next page loads.

There is some concern with the length of the token if we add too much data: T239680#5868252, but it seems we're not at the limit yet.

Should we make persistence of highlights across the pages a requirement for this task?

I'm worried that the complexities that keep cropping up around using this token method might overwhelm the value of using that particular method. What would it look like if we just used the POST as we'd previously described? I feel like these kinds of changes become much easier with the POST.

I need some help understanding the pros of using the token instead of the POST given these complexities.

@aezell We could also persist the highlighting across pages if we were using POST. We'd do that via hidden fields, similar to the 'offset', etc here: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CheckUser/+/561890/8/modules/ext.checkuser.specialInvestigate.js

dmaza added a subscriber: dmaza.Feb 24 2020, 10:32 PM

@Tchanders To confirm - sticky highlights persist across paginated tabs right?

@Niharika Not in the current patch. We could do this by adding some extra information to be passed via the token: i.e. the data that should be highlighted when the next page loads.

Why can't we do it using local storage? It would mean that you will have to add the pin button to the first match in the new page so to allow the user to "unpin" but other than that I don't see a problem. Am I missing something?

dbarratt added a comment.EditedFeb 24 2020, 10:35 PM

There is some concern with the length of the token if we add too much data: T239680#5868252, but it seems we're not at the limit yet.

I feel like we are a long ways from the 2,000 character limit, and if we (prematurely) drop IE support in this extension, we have effectively no limit.

Obviously it is something we should be mindful of, but I can't imagine this ever being a problem for the vast majority of our users.

I need some help understanding the pros of using the token instead of the POST given these complexities.

I think the biggest con is that the links are no longer functionally links, they are buttons. We could change them to be buttons, but styled like links (which would resolve the accessibility concerns).

I didn't do a whole lot of testing with the patch, but I worry things like a control+click (to open in a link in a new tab) wouldn't work properly. (and afaik, there isn't a way to open a new tab with a POST in JS) This could be fixed by setting the target attribute on the form when the form is submitted.

I'm also not sure how all of the browsers we support deal with multiple POST requests in the browser history. It seems like some will store a number of requests in their cache, but how large that cache is might be worth testing. It might also be good to know which ones don't have any sort of POST cache in the history (if any).

I can do a deeper dive into this if you would like and get more specifics on what would break where, but the token feels like fewer issues overall.

I would prefer to just put the data in plaintext the URL (which is encrypted anyways), but because of the logs, that seemed like a non-starter. I wonder if there is a way we could have the logs filter out query strings on these titles? For instance if a page is /wiki/Special:Investigate* or index.php?title=Special:Investigate* it would remove anything else from the query string?

Why can't we do it using local storage? It would mean that you will have to add the pin button to the first match in the new page so to allow the user to "unpin" but other than that I don't see a problem. Am I missing something?

Oh! that's a great idea. :) Let's do that.

Why can't we do it using local storage?

This is a good question. It did cross my mind, but there are a few issues:

  • since local storage is shared across tabs, the highlighting would be the same across all open investigations (we could perhaps make a unique ID for each investigation, but we're wandering into the territory of making maintenance more difficult)
  • we'd need to be careful not to pollute the store (e.g. making sure we delete everything if the user closed the tab without removing the highlight)
  • window.sessionStorage would be an alternative, but this wouldn't be able to be deliberately persisted across tabs (e.g. if there were multiple pages open from the same investigation)

@aezell An even better solution to the state problem might be to create a "Case" (in a new database table? or a new field on the existing log table?). The case could serve as the log T245662#5905295, but it could also store the state of the investigation. That way, you'd submit an investigation and get redirected to /Special:Investigate/CASEID which keeps the URLs super short. :)

It would mean that you will have to add the pin button to the first match in the new page so to allow the user to "unpin" [...]

@dmaza This is also a good point, which we should bear in mind whichever way we persist the highlighting across pages.

@Niharika Should we add persisting highlights across pages to the acceptance criteria?

@Niharika @Prtksxna I have a few questions that arose while working on this:

  • Are the round buttons with a white background a literal design? For now I've kept the OOUI design for the toggleButtonWidget. It seems sensible to have the buttons and colours look consistent with the other OOUI components on the site, so that it's clear these are buttons, and clear when they are toggled on/off.

No they aren't a literal design. The ToggleButtonWidget that you've picked seems to be the most appropriate for this function. 👍🏽

  • As we've discussed somewhere before, having buttons that appear and disappear means the rows keep changing height (this is also the case on the prototype). Should we have another task to address this and any other design issues that crop up after seeing the working product?

Yeah a separate task makes sense. Once this is implemented I want to see how much height the row takes up when the buttons are visible, and if it makes sense to keep that height all the time. This might negatively affect the density of information though.

  • Having the pin buttons disappear after a row is pinned makes the UI complicated, because if the check user then wants to unpin that highlight, they may have to look through several rows to find which button they pinned.

I am not completely sure I understand this situation. From the notes in the description: "For the above example, all Chrome 65 cells would have had the pin highlighted once it was selected. This way the user can unpin from any highlighted row and doesn't have to find the original row/cell they highlighted from". This would mean that a check user can unpin from any highlighted row. Maybe I am understanding this incorrectly though, could you elaborate.


Am I right in understanding from your comment that it's possible to do multiple highlights at once? In that case, are there any technical reasons to introduce a limit on number of highlights allowed?

Ah, I don't remember the discussion - I just assumed multiple highlight pinning was required. The current patch lets you pin multiple highlights (but no two cells with the same data). Technically it's fine - we just add classes to all the affected table cells. If that's not what we want design-wise, it can be changed.

I'd be wary of allowing multiple highlight at the moment for two reasons:

  1. The workflow we are imagining is of highlighting and filtering. Ideally, this would be a repeated process of elimination and not of managing multiple options (I could be wrong about this, we'll find out as people use the feature).
  2. If we do chose to add multiple highlights we'll need more design and testing around it. All highlights in the same color might not be the best way to present this information. Providing this feature would require more work.

@Prtksxna Thanks for the detailed responses - I think most of the confusion stemmed from me thinking that we were allowing multiple highlights.

I am not completely sure I understand this situation. From the notes in the description: "For the above example, all Chrome 65 cells would have had the pin highlighted once it was selected. This way the user can unpin from any highlighted row and doesn't have to find the original row/cell they highlighted from". This would mean that a check user can unpin from any highlighted row. Maybe I am understanding this incorrectly though, could you elaborate.

This was quite a confusing UI with multiple highlights allowed, but I can see how it's much more straightforward with just one. Will update the patch.

I'd be wary of allowing multiple highlight at the moment [...]

No problem - will update the patch.

dmaza added a comment.Feb 25 2020, 3:45 PM

Why can't we do it using local storage?

This is a good question. It did cross my mind, but there are a few issues:

  • since local storage is shared across tabs, the highlighting would be the same across all open investigations (we could perhaps make a unique ID for each investigation, but we're wandering into the territory of making maintenance more difficult)
  • we'd need to be careful not to pollute the store (e.g. making sure we delete everything if the user closed the tab without removing the highlight)
  • window.sessionStorage would be an alternative, but this wouldn't be able to be deliberately persisted across tabs (e.g. if there were multiple pages open from the same investigation)

Either option (localStorage, sessionStorage) works for a first iteration IMO. I think you can work out some logic and have a combination of both so to be able to share and expire the highlight data if that's something we are concerned about.

My opinion is that the highlighting should be as ephemeral as possible. It seems like that would be the easiest for the user to reason about. That is, if I have to remember that opening a new tab sometimes keeps highlights and sometimes doesn't depending on which link I click on, that's a lot of overhead for the user. Instead, just making it as simple as possible, if slightly less effective, seems like a less problematic path.

Can the page know the difference between being loaded in a new tab and simply being a refresh? I don't think so but maybe browsers make this possible now. If so, we could potentially look for that to determine if we should destroy and recreate the highlighting.

Can the page know the difference between being loaded in a new tab and simply being a refresh? I don't think so but maybe browsers make this possible now. If so, we could potentially look for that to determine if we should destroy and recreate the highlighting.

I think the problem is... do you expect to have the same things pinned on multiple investigations or should they be unique (per-investigation)?

I think the answer to that is the latter. It seems to me that the highlighting is informed by a reading of the results by the investigator. Those hunches or discernments would change for each investigation as the results would be different.

Since we're still discussing this (and probably estimated before considering the persistence across pages), should we make persistence across pages into a separate task?

That makes sense to me. It'll make it easier to estimate both (or reestimate if we choose) and make the acceptance criteria clearer.

It would mean that you will have to add the pin button to the first match in the new page so to allow the user to "unpin" [...]

@Niharika Should we add persisting highlights across pages to the acceptance criteria?

I saw your latest comment about making that a new task. Created T246172: CU 2.0: Persist sticky highlights across paginated tabs [Medium] for that.

Can the page know the difference between being loaded in a new tab and simply being a refresh? I don't think so but maybe browsers make this possible now. If so, we could potentially look for that to determine if we should destroy and recreate the highlighting.

I think the problem is... do you expect to have the same things pinned on multiple investigations or should they be unique (per-investigation)?

I agree with Alex's answer to this. Every investigation is unique and different things will be highlighted at different times.

  • As we've discussed somewhere before, having buttons that appear and disappear means the rows keep changing height (this is also the case on the prototype). Should we have another task to address this and any other design issues that crop up after seeing the working product?

Yeah a separate task makes sense. Once this is implemented I want to see how much height the row takes up when the buttons are visible, and if it makes sense to keep that height all the time. This might negatively affect the density of information though.

Task filed here: T246294

Change 573421 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Add highligting on hover to Special:Investigate results tables

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

dom_walden added a subscriber: dom_walden.

As this is still in its early stages, haven't tested much apart from basic functionality. Highlighting works, and highlights all other cells that match.

Currently, highlighting is based on cells matching exactly. For example, on Preliminary Check the "Date attached" is to the nearest second, so normally only one cell highlights at a time. Similarly UA strings need to match exactly.

I haven't tested this cross-browser or on a mobile (yet), but will probably have to at some point.

There are some UX issues, as noted in T246294.

  • We wont be putting the funnel button right now
  • There might be some changes to just the buttons later (size, icon etc)
  • Once the user moves their mouse from the cell the buttons go away, but the highlight remains
  • Hovering on the cell again brings back the button, and that is how you unpin it

At the moment the button remains, as Thalia notes in T237299#5907053.

  • For the above example, all Chrome 65 cells would have had the pin highlighted once it was selected. This way the user can unpin from any highlighted row and doesn't have to find the original row/cell they highlighted from

You can unpin by clicking any of the buttons.

  • All other highlighting stops if a highlight has been pinned (we might want to change this in the future?)

Yep.

For rows that are not currently highlighted, there is still a slight darkening of the background colour of the entire row if you hover over it. This is a different colour to the pinned highlighting, or the highlighting when nothing is pinned.

For example,

Thanks @dom_walden.

Currently, highlighting is based on cells matching exactly. For example, on Preliminary Check the "Date attached" is to the nearest second, so normally only one cell highlights at a time. Similarly UA strings need to match exactly.

Have asked about this in T246294#5926253.

At the moment the button remains, as Thalia notes in T237299#5907053.

Added to T246294

For rows that are not currently highlighted, there is still a slight darkening of the background colour of the entire row if you hover over it. This is a different colour to the pinned highlighting, or the highlighting when nothing is pinned.

Added to T246294

Niharika closed this task as Resolved.Feb 28 2020, 8:20 PM

Okay, carrying on the design work remaining in T246294. Closing this one.