Page MenuHomePhabricator

Highlight on VE not covering full button area
Closed, ResolvedPublic

Description

steps to reproduce

  1. Go to any page with VE enabled
  2. Highlight the Paragraph selector

Observed:

Expected:
Highlight should cover the full area

Event Timeline

ovasileva triaged this task as Medium priority.Jul 31 2020, 8:27 AM
ppelberg moved this task from To Triage to Triaged on the VisualEditor board.
Jdlrobson edited projects, added Vector (Vector (Tracking)); removed Vector.EditedSep 3 2020, 10:36 PM
Jdlrobson added a subscriber: nray.

The top padding of the content aligns just right with the labels in the toolbar:

In new Vector the padding changes from 1em to 1.25em.

A rule in VisualEditor seems to be based on this value:

.ve-init-mw-desktopArticleTarget-toolbar, .ve-init-mw-desktopArticleTarget-toolbarPlaceholder {
  margin:  -1.14em -1.14em 1.14em -1.14em;
}

The margin top value -1.14em seems to be based on this value and needs adjusting to -1.44em -1.14em 1.14em -1.14em
I would suggest future proofing this value in case it changes again, but these seem like changes for VisualEditor not Vector.

Yes, our integration styles are specifically targeting the existing margins in Vector (and other skins). I assume your team is still going to work?

Is there a way we can revisit these styles inside VisualEditor? Right now we're tweaking these a lot so I can imagine us wasting a lot of time keeping these in sync. If we are okay with the bug, we can also wait until the design for new Vector has been finalized.

In the interest of better skin support and better maintainability is it feasible that this could be absolutely positioned with a fixed height? - I recall we had lots of problems with Minerva for the same problem.

Alternatively, we could find a way to provide a LESS variable which VisualEditor can use as part of a skinStyle but that might need some platform changes to ResourceLoader. That would at least make these calculations more future proof.

Is there a way we can revisit these styles inside VisualEditor? Right now we're tweaking these a lot so I can imagine us wasting a lot of time keeping these in sync. If we are okay with the bug, we can also wait until the design for new Vector has been finalized.

In the interest of better skin support and better maintainability is it feasible that this could be absolutely positioned with a fixed height? - I recall we had lots of problems with Minerva for the same problem.

Alternatively, we could find a way to provide a LESS variable which VisualEditor can use as part of a skinStyle but that might need some platform changes to ResourceLoader. That would at least make these calculations more future proof.

I'm happy to review an alternative approach if you would find that easier. I would suspect that changing the toolbar's positioning would be much more work given the complexity we have around dynamic floating. Either way the regression should be fixed, as this is currently being shown to real users on various wikis.

Catrope added a subscriber: Catrope.EditedSep 8 2020, 7:53 PM

Alternatively, we could find a way to provide a LESS variable which VisualEditor can use as part of a skinStyle but that might need some platform changes to ResourceLoader.

Said platform changes are patiently awaiting review at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/434211/ . I'll ping James to ask him to review it again.

Change 628173 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Allow VisualEditor direct access to variables used for padding content

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

It's my understanding that the above patch will unblock editing team by allowing them access to the padding values for their own calculations.
Please confirm, and I'll move this into our code review process.

Discussed this with @ppelberg yesterday and I think we can drop the priority for now and try to fix it a bit later on in the project. (cc @alexhollender, @iamjessklein)

ovasileva lowered the priority of this task from Medium to Low.Sep 23 2020, 9:33 AM

Change 633828 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Move local variables to skin variables so VisualEditor can read them

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

Change 633834 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/VisualEditor@master] [vector modern] VisualEditor margins are tied to Vector variables.

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

Change 628173 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use variables for padding content

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

Change 635046 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Define padding-top-content and padding-horizontal-content variables

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

@Esanders The clean solution in the long term is to move .oo-ui-toolbar up 2 levels in the DOM into .mw-content-container (which has the right dimensions), just above main#content.mw-body and completely remove the margin rule.
This however only works for the new Vector yet, all old skins, including Timeless need some custom margin-left in that spot. Nevertheless that's technical debt and should be eliminated for future skins.

Change 633834 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] [vector modern] VisualEditor margins are tied to Vector variables.

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

Change 635046 abandoned by Jdlrobson:
[mediawiki/core@master] Define padding-top-content and padding-horizontal-content variables

Reason:
Let me know if I should restore this. Abandoning to restore a healthy workload level.

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

Change 633828 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] Move local variables to skin variables so VisualEditor can read them

Reason:
Let me know if I should restore this. Abandoning to restore a healthy workload level.

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

Jdlrobson reassigned this task from Esanders to ovasileva.Fri, Feb 19, 3:37 AM
ovasileva closed this task as Resolved.Fri, Feb 19, 9:57 AM

Looks like it! Resolving.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFri, Feb 19, 9:57 AM