Page MenuHomePhabricator

New Vector: The footer text is showing through the link inspector suggestions dialog inside Reply tool
Closed, ResolvedPublic2 Estimated Story Points

Description

Steps to reproduce:

  1. Go to the bottom of a discussion page.
  2. Click on Reply link on the last comment.
  3. Open link inspector>Wikipedia.
  4. Type a letter to open up the suggestion drop down.

Observed Result: Observe that, the text on the footer is showing through the link suggestions on the link inspector dialog.

Screenshot/Screen capture:

Screen Shot 2020-10-05 at 3.09.10 PM.png (1,214×1,494 px, 328 KB)

Browser: Chrome

Wiki: English

Page: https://en.wikipedia.beta.wmflabs.org/wiki/User_talk:RYasmeen_(WMF)/sandbox

Environment: Beta cluster.

Event Timeline

matmarex subscribed.

Old Vector works correctly, only new Vector has the bug:

image.png (3,238×2,020 px, 684 KB)

image.png (3,238×2,020 px, 632 KB)

Esanders added subscribers: JTannerWMF, Esanders.

I think the reading team should try to fix this in vector first. Cc @JTannerWMF

It's probably something complicated to do with stacking contexts. This rule causes it:

.skin-vector-max-width.skin-vector-search-header .mw-workspace-container {
    position: relative;
}

image.png (3,238×2,020 px, 732 KB) image.png (3,238×2,020 px, 701 KB)

We probably should fix this in our code though. Although it worked as expected in all other skins, I think we just got lucky, I don't the skin authors intentionally made that work.

Esanders renamed this task from [Regression pre-wmf.11] The footer text is showing through the link inspector suggestions dialog inside Reply tool to New Vector: The footer text is showing through the link inspector suggestions dialog inside Reply tool.Oct 6 2020, 12:21 AM

The Web team will investigate what is going on here and touch base with Editing

Jdlrobson added subscribers: nray, Jdrewniak, Jdlrobson.

summary

This likely requires a change in Vector by reading web team as a short-term fix but I need input from @nray and @Jdrewniak

I'd recommend future-proofing this code with a change in OOUI to always append overlays to the body element otherwise errors like this are likely to pop up in other places. FYI this is also broken in a slightly different way on Minerva skin (https://en.wikipedia.beta.wmflabs.org/wiki/Talk:Sandbox?useskin=minerva)

analysis

Interestingly this doesn't happen for VE.

Screen Shot 2020-10-06 at 10.37.35 AM.png (1,658×1,224 px, 280 KB)

Looking more closely it seems like the overlay .ve-ui-overlay is being appended to a dd element inside .mw-parser-output in the case of structured discussions and #bodyContent in the case of VisualEditor. I recall we had similar issues with mobile Overlay which culminated in us always appending overlays to the body tag. Is that something that's an option for OOjs UI as a long term solution?

As a short term solution, I don't think bumping the z-index for mw-parser-output is an option here - that element can contain user generated content and create challenges with the UI e.g. blocking user interface elements.

@nray can we remove the position relative on .skin-vector-max-width.skin-vector-search-header .mw-workspace-container ? There is an inline comment "// The sidebar is absolutely positioned relative to the mw-workspace-container." but I'm not seeing any impact on the design without it.

The reason this is happening is that when position: relative is set on an element then a new containing block is established and z-index only applies to other elements inside the same block. We are setting position: relative in the new version of Vector for the sidebar. @Jdrewniak you're our resident z-index spec expert.. if we can't remove the position:relative, what options do we have here?

@Jdlrobson

I put the position: relative style on that .mw-workspace-container element because in the future I wanted to remove .mw-page-container-inner element/styles. That element currently has print styles that I didn't want to remove as part of T261802, but .mw-page-container-inner's original purpose was to make it easier to absolutely positioning everything prior to T261802 and is therefore obsolete IMO. The sidebar is still absolutely positioned and is reliant on the relative positioning of either .mw-workspace-container or .mw-page-container-inner to maintain its correct position (if you turn off the relative positioning on both of those elements, you will notice the sidebar gets clipped).

I agree the long-term solution is to move that overlay to be a direct child of the body. Removing .mw-workspace-container's relative positioning and keeping .mw-page-container-inner's relative positioning could work short-term, but as an alternative I noticed setting mw-footer-container to position: static seemed to work as well. The footer also has the .mw-workspace-container class and thus is also currently relatively positioned which I think contributes to this problem.

This element doesnt have the same issue:

Screen Shot 2020-10-15 at 11.18.12 AM.png (1,488×1,120 px, 215 KB)

It uses an element .oo-ui-defaultOverlay which is a child of body.

This one uses oo-ui-windowManager - shouldn't that also be positioned in the same place in the DOM?

@Esanders The right solution seems to be to place the windowManager in the body tag inside OOUI. Could you point me to the code which insert oo-ui-windowManager into the DOM?

Change 634329 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] Apply static positioning to mw-footer-container

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

^^ Above patch is the "quickfix". I agree with @Jdlrobson that appending the popover at the end of the body is the more resilient solution though to these types of regressions . If we go that route, we should probably re-estimate this as it will probably be more work

\cc @ovasileva

Change 636523 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] [WIP] CommentTarget: Use an overlay

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

Looking more closely it seems like the overlay .ve-ui-overlay is being appended to a dd element inside .mw-parser-output in the case of structured discussions and #bodyContent in the case of VisualEditor. I recall we had similar issues with mobile Overlay which culminated in us always appending overlays to the body tag. Is that something that's an option for OOjs UI as a long term solution?

I agree, this is the proper solution. Some OOUI widgets have a config option called $overlay, which basically does this, but that can't be easily reused for VisualEditor's code, as there's a lot of bespoke stuff for positioning the contexts and inspectors relative to cursors and selections.

You're lucky though because I investigated doing that about three years ago, with the goal of fixing T74121, and I apparently still have the code I played with then, and it seems to work after rebasing it. On the other hand, I don't remember exactly how it works, and the fact that I didn't upload it for review back then tells me it has to have some bugs.

I just pushed that for review; I guess we can try it out.

Also I filed a subtask for that work (T266521), and connected it to separate tasks for applying that fix to DiscussionTools (this task), Flow (I just tested and it has the same issue in new Vector: T266520), and VisualEditor dialogs (T74121).

Change 634329 abandoned by Nray:
[mediawiki/skins/Vector@master] Apply static positioning to mw-footer-container

Reason:
I believe the editing team is taking on this ticket now

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

Change 634329 restored by Esanders:
[mediawiki/skins/Vector@master] Apply static positioning to mw-footer-container

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

@matmarex has the downstream fix on his radar, but it's a lot more complicated. For the time being we'll go with the Vector workaround.

Change 634329 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Apply static positioning to mw-footer-container

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

matmarex moved this task from Code Review to QA on the Editing-team (Kanban Board) board.
matmarex added a project: Editing QA.
ppelberg claimed this task.

Change 636523 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/DiscussionTools@master] [WIP] CommentTarget: Use an overlay

Reason:

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