Page MenuHomePhabricator

Performance review 2015: mediawiki.page.gallery module
Open, LowPublic

Description

Mainly introduced in:

  1. Lacking documentation with regards to intended behaviour.
  1. Buggy user-experience. The two resize handles contradict each other. On resize, three visible actions happen:
    • Native browser rendering updated based on the distribution of the css-floated elements.
    • First resize handle fires, which resizes the images back to their original size. This causes other content to reflow.
    • Second resize handle fires, which resizes the images again (to the destination size).

E.g. Given a gallery with images that fit on one row for the current window size. Widen the window. The module is then supposed to widen the images to fill the extra space. The first resize handler makes the images smaller (original size), the second resize handle makes them bigger. Aside from it being plain wrong that the images are first being made smaller (instead of bigger), it shouldn't be doing the resize in two steps. It's fine to do some pre-computation in an earlier handler (before the 300ms debounce), but it shouldn't be causing an active reflow in both.

  1. Poor javascript performance.

It adds lots of inline style attributes, duplicates thereof and doesn't account for JavaScript's number type being double-precision float – thus creating lots of fractions. The img element itself has a strict integer value for the width/height, so the fractions are trimmed there by the browser.

<div style="width: 503.9604743083px;">
  <div class="thumb" style="width: 501.9604743083px;">
    <div style="margin: 0px auto;"><a ...><img alt="" src=".../513px-VisualEditor-logo.svg.png" width="501" ...>
  </div>
  <div class="gallerytextwrapper" style="width: 481.9604743083px;">
    ...
  </div>
</div>

Event Timeline

Krinkle created this task.Feb 5 2015, 5:05 AM
Krinkle raised the priority of this task from to Needs Triage.
Krinkle updated the task description. (Show Details)
Krinkle added subscribers: Krinkle, Bawolff, Fomafix and 2 others.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 5 2015, 5:05 AM
Krinkle set Security to None.
Aklapper triaged this task as Low priority.Feb 5 2015, 1:14 PM
  1. Buggy user-experience. The two resize handles contradict each other. On resize, three visible actions happen:
    • Native browser rendering updated based on the distribution of the css-floated elements.
    • First resize handle fires, which resizes the images back to their original size (why?).
    • Second resize handle fires, which resizes the images again (to the destination size).

The rationale for this behavior was convincingly explained to me by @m4tx on 3768d9a620cda02e163b69b282bdd66e60e6ae90 (https://gerrit.wikimedia.org/r/#/c/182652/):

I think that restoring original sizes in debounce with at_begin=true and justify() in debouce with at_begin=false is better. In this case, user first sees as the images scale down, they resize the window, and when they stop it, then they are scaled up, but not rearranged into different rows. In case of doing both steps with at_begin=false, the items would be actually rearranged (generally it would look more random).

Krinkle renamed this task from Clean up issues with mediawiki.page.gallery.js to UX and performance issues with mediawiki.page.gallery module.Feb 9 2015, 8:38 PM
Krinkle updated the task description. (Show Details)Feb 9 2015, 8:58 PM
Jdforrester-WMF moved this task from Untriaged to Backlog on the Multimedia board.Sep 4 2015, 5:53 PM
Restricted Application added a subscriber: Matanya. · View Herald TranscriptSep 4 2015, 5:53 PM
Krinkle renamed this task from UX and performance issues with mediawiki.page.gallery module to Performance review 2015: mediawiki.page.gallery module.May 9 2018, 4:38 PM
Krinkle updated the task description. (Show Details)
Imarlier assigned this task to Krinkle.Jun 25 2018, 8:06 PM
Imarlier moved this task from Inbox to Next In This Quarter on the Performance-Team board.
Imarlier added a subscriber: Imarlier.

@Krinkle is going to re-evaluate whether the refactor makes this irrelevant.

Change 445567 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.page.gallery: Various clean up and minor optimisations

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

@matmarex I didn't see your comment in 2015. Reading it now, I'd like to better understand what you meant. I can't quite figure out why the double resize would be desirable. I do understand that the min-height trick avoids duplicate reflows already, which I suppose we can keep.

But, I do not understand the benefit from the initial snap to the original size.

From reading the code, it seems that the only purpose of the duplicate sizing is, to make the implementation of justify() work, which currently depends on being able to (needlessly?) re-compute the image dimensions and expects the original size (instead of using the the cached size it already has access to?). This explains why the initial resize handler changes the size back, to make that logic work. But if that is the only reason, that seems like something we can fix.

However, as I said, I didn't understand M4tx's explanation, and I trust your judgement saying this is intentional.

When I try to hack it up locally (removing the handleResizeStart handler and calling it from handleResizeEnd), the experience seems better at first glance. The main issue I found is that rows don't have the same height, which means when resizing in a way that makes images go to a different row, during the first 0.3s there will be images on the same row with a different height. I haven't thought this through, but that seems fixable? Maybe there's a paradox here that I haven't found yet.

Change 445567 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.page.gallery: Various clean up and minor optimisations

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

Krinkle removed Krinkle as the assignee of this task.Jul 18 2018, 1:43 AM