[go: up one dir, main page]

Page MenuHomePhabricator

[wmf.29] [mobile] Section level image placeholder displays an additional visual element
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue

  • Go to Homepage on cswiki and select "Add an image an article section" task
  • Go to any of the suggested articles - the placeholder for a suggested image will have an additional visual element displayed underneath:

Examples

Screenshot 2023-11-13 at 17.36.10.png (1×1 px, 360 KB)
Screen Shot 2023-10-06 at 4.57.27 PM.png (1×618 px, 201 KB)

Relevant node: <div class="ve-ce-leafNode ge-section-image-placeholder ge-align-right oo-ui-image-progressive oo-ui-icon-image" style="width: 0px; height: 130px;"></div>

Event Timeline

KStoller-WMF renamed this task from [wmf.29] [mobile] Section level image placeholder disaplays an additional visual elment to [wmf.29] [mobile] Section level image placeholder displays an additional visual element.Oct 13 2023, 4:49 AM
KStoller-WMF triaged this task as Medium priority.
KStoller-WMF moved this task from Inbox to Backlog on the Growth-Team board.

I'll investigate this, since it happens deterministically.

Preliminary inestigation: I tested this at Czech Wikipedia, and I can confirm the behaviour, with one difference: I see no placeholder at all:

cswiki bug.png (2×1 px, 478 KB)

Further checks show that both lines actually represent placeholder with width: 0 set on them. Fixing that in the developer tools shows two placeholders:

cswiki_bug 2.png (2×1 px, 488 KB)

The good question is why this happens, but this is definitely important for us to work on.

For some reason,this.model.getAttribute( 'width' ) seems to be evaluating to 0. I do not understand why though (or when that changed). I asked @Sgs to have a look.

I updated

For some reason,this.model.getAttribute( 'width' ) seems to be evaluating to 0. I do not understand why though (or when that changed). I asked @Sgs to have a look.

That's the issue making the placeholder narrow; the height is also evaluating to 0 since its derived from the width in AddImageArticleTarget#275 but the fallback is preventing the placeholder height to shrink. I have no clue yet what has changed in VE/GE/OOUI to make this happen. Maybe some markup has changed making the this.surface.getView().$documentNode element no longer have a computed width? Or we're running into some initialization race I cannot see yet. Insight from Editing-team would be appreciated to identify what changed to break the placeholder logic. cc @Esanders @matmarex

Regarding the additional visual element inserted I think it relates to the fact we're creating two instances of RecommendedImageToolbarDialog and that calls insertImagePlaceholder twice resulting in two (broken) placeholders. The issue seems to be present in all structured tasks as the double instantiation comes from the ve.loadModules hook handler firing more than once. Did something change in MobileFrontend to fire more than one ve.loadModules event? cc @Jdlrobson

Change 973834 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Section images: avoid inserting broken and duplicated placeholder

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

(…) I have no clue yet what has changed in VE/GE/OOUI to make this happen. Maybe some markup has changed making the this.surface.getView().$documentNode element no longer have a computed width? Or we're running into some initialization race I cannot see yet. Insight from Editing-team would be appreciated to identify what changed to break the placeholder logic. cc @Esanders @matmarex

I've placed a breakpoint on this line: https://gerrit.wikimedia.org/g/mediawiki/extensions/GrowthExperiments/+/6344ef5d492ddcf33b5ca62e796d28f5bfa9c6b2/modules/ext.growthExperiments.StructuredTask/addimage/AddImageArticleTarget.js#269 and found that it has no width because it's not displayed at all at this point, the whole overlay is hidden with display: none.

It definitely seems like something has changed about the order of initialization. Changes from T239676 may be the culprit (I didn't try to check, but it's the only thing I'm aware of).

But, taking a step back, it seems to me that measuring the width of the screen to set the size of the placeholder isn't that great. Wouldn't it result in incorrect rendering if the user rotates their phone between portrait and landscape after opening the editor? I would suggest using the default thumbnail size, just like on desktop, and then setting max-width: 100% in CSS, just like MobileFrontend does for real images.

(by the way, I'm on the MediaWiki Platform team now :) )

(…) The issue seems to be present in all structured tasks as the double instantiation comes from the ve.loadModules hook handler firing more than once. (…)

FWIW, I wasn't able to reproduce that, and I didn't see the doubled-up placeholders.

I've placed a breakpoint on this line: https://gerrit.wikimedia.org/g/mediawiki/extensions/GrowthExperiments/+/6344ef5d492ddcf33b5ca62e796d28f5bfa9c6b2/modules/ext.growthExperiments.StructuredTask/addimage/AddImageArticleTarget.js#269 and found that it has no width because it's not displayed at all at this point, the whole overlay is hidden with display: none.

It definitely seems like something has changed about the order of initialization. Changes from T239676 may be the culprit (I didn't try to check, but it's the only thing I'm aware of).

Reverting the change makes the placeholder get back its width so there's definitely something with it.

But, taking a step back, it seems to me that measuring the width of the screen to set the size of the placeholder isn't that great. Wouldn't it result in incorrect rendering if the user rotates their phone between portrait and landscape after opening the editor? I would suggest using the default thumbnail size, just like on desktop, and then setting max-width: 100% in CSS, just like MobileFrontend does for real images.

That's true, the image is not resized neither with js or css if there's an orientation change, however it's right aligned to the text when the width reaches the tablet min-width breakpoint. On desktop we use VE's thumbLimits and the user preference, I'm not sure why on mobile we capped the width to 500px. It seems pretty random to me but I'm leaving it in place just in case.

(by the way, I'm on the MediaWiki Platform team now :) )

Ops, thanks for taking the time to reply here. Good luck in your new team:)

(…) The issue seems to be present in all structured tasks as the double instantiation comes from the ve.loadModules hook handler firing more than once. (…)

FWIW, I wasn't able to reproduce that, and I didn't see the doubled-up placeholders.

I'm also unable to reproduce it in production wikis but it seems consistent in the development setup. @Urbanecm_WMF @Etonkovidova are your screenshots coming from production wikis or local setup?

FWIW, I wasn't able to reproduce that, and I didn't see the doubled-up placeholders.

I'm also unable to reproduce it in production wikis but it seems consistent in the development setup. @Urbanecm_WMF @Etonkovidova are your screenshots coming from production wikis or local setup?

Re-tested in cswiki wmf.5 (and eswiki) - I see it consistently:

Screen Shot 2023-11-21 at 11.13.30 AM.png (1×882 px, 262 KB)
after clicking on Yes to add the image:
Screen Shot 2023-11-21 at 11.15.34 AM.png (1×822 px, 518 KB)

The initial screenshots in the task description came from the production.

Sgs raised the priority of this task from Medium to High.Nov 27 2023, 12:31 PM

I think the changes from T334263 could explain the miss-behavior. If I revert 931501 the placeholder is inserted only once.

It seems the overlayManager.add callback in setupEditor() is being called twice; one coming from a OverlayManager docReady handler and the second from OOUI router hashchange listener. That leads to double call of mw.libs.ve.targetLoader.loadModules firing repeated ve.loadModules events and causing double registration of GrowthExperiments' plugins on mobile.

If I defer the router.navigate call eg: util.docReady( function () { router.navigate( fragment ); } ); the overlay callback is only triggered once because the path has not yet been set when OverlayManager docReady handler kicks in. But I'm not sure if there are unintended consequences of doing that.

@DLynch @Jdlrobson does this make any sense? I've submitted a tentative patch but maybe you can provide guidance on a preferred solution? Maybe the OverlayManager docReady handler is not needed anymore?

Change 978117 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/MobileFrontend@master] mobile.init: avoid hash changes before document loads

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

Change 978117 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] mobile.init: avoid hash changes before document loads

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

The fix works in beta.

The issue is still present in wmf.9 since the fix for T352208: [wmf.29] [mobile] Section level image placeholder has no width will be deployed for wmf.10.

Checked in wmf.10 for arwiki and cswiki - works as expected. Note: the minor issue on narrow screens - https://gerrit.wikimedia.org/r/983761 - will be deployed in wmf.12.