steps to reproduce
- Go to any page with VE enabled
- Highlight the Paragraph selector
Observed:
Expected:
Highlight should cover the full area
ovasileva | |
Jul 31 2020, 8:24 AM |
F34113110: Screen Shot 2021-02-18 at 7.32.20 PM.png | |
Feb 19 2021, 3:32 AM |
F32244014: Screen Shot 2020-09-03 at 3.30.29 PM.png | |
Sep 3 2020, 10:36 PM |
F31955315: Screen Shot 2020-07-31 at 10.23.19 AM.png | |
Jul 31 2020, 8:24 AM |
Observed:
Expected:
Highlight should cover the full area
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | ovasileva | T258072 [REQUEST] QA Support on the new header | |||
Duplicate | • alexhollender_WMF | T259083 [Regression] UI regressions in VE on Beta cluster | |||
Resolved | ovasileva | T259331 Highlight on VE not covering full button area |
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.
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.
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
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)
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
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.
Change 628173 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use variables for padding content
Change 635046 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Define padding-top-content and padding-horizontal-content variables
@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.
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.
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.