Page MenuHomePhabricator

Fix behavior of toolbar buttons in new template editor sidebar
Open, Needs TriagePublic2 Estimated Story Points

Description

The toolbar buttons at the bottom of the sidebar still behave weird in some situations. This is a placeholder ticket for everything related when it doesn't fit into another ticket in the sprint.

Do not move this ticket to the next sprint, but create new ones.

  • Sidebar selection determines which toolbars buttons are active.
    • New template and new wikitext buttons are always enabled
    • When a wikitext or template header is selected, up/down and delete buttons are enabled.
    • When a parameter is selected, the up/down and delete buttons are disabled. Uh, no. Why? @awight?
      • Bug: click back and forth between parameters. Sometimes the buttons are disabled, sometimes enabled. Maybe a race condition between selecting the parameter and selecting the template part?
  • Up/down are disabled for the first and last parts, respectively.
  • No matter how the user arrives at a given selection (tab, content click, sidebar click, deleting another parameter...), they see the same buttons enabled.
    • Hard to tell until the other bug is resolved.
  • Nothing changes in the old workflow
  • (nice-to-have) No flicker
    • Currently the up/down/delete buttons flicker disabled for every change. Probably related to the bug above.

Event Timeline

Change 720773 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Make up/down/remove buttons behave sane on \u2026AddParameterPage

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

thiemowmde set the point value for this task to 1.
thiemowmde moved this task from Backlog to In sprint on the WMDE-Templates-FocusArea board.
thiemowmde moved this task from Proposed to Currently in sprint on the WMDE-TechWish board.
thiemowmde moved this task from Sprint Backlog to Review on the WMDE-TechWish-Sprint-2021-09-15 board.

Change 721471 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Regression: Fix broken click on top-level template elements

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

Change 721471 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Regression: Fix broken click on top-level template elements

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

Change 721846 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Regression: Update selection in new sidebar relative to content

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

Change 721846 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Regression: Update selection in new sidebar relative to content

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

Change 720773 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Make up/down/remove buttons behave sane on \u2026AddParameterPage

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

awight changed the point value for this task from 1 to 3.
awight changed the point value for this task from 3 to 2.

Tracing shows that the outline controls are first set correctly, but then another chain of 6 events through onOutlineSelectWidgetSelect results in the entire part (template) being selected.

console.trace() update outline: !isMovable !isRemovable 
abilities { move: true, remove: true }
 
Object { elementId: null, visible: true, data: "part_3/ZWEIGLEISIG", "$element": {…}, elementGroup: {…}, bindings: {}, disabled: false, wasDisabled: false, "$label": {…}, label: "ZWEIGLEISIG", … }
OutlineControlsWidget.js:148:6
    onOutlineChange OutlineControlsWidget.js:148
    emit oojs.js:901
    selectItem SelectWidget.js:821
    selectItemByData SelectWidget.js:758
    focusPart ve.ui.MWTemplateDialog.js:649
    emit oojs.js:901
    emit oojs.js:901
    onTemplateParameterChoose ve.ui.MWTransclusionOutlineTemplateWidget.js:199
    emit oojs.js:901
    chooseItem SelectWidget.js:880
    onDocumentMouseUp SelectWidget.js:262


console.trace() update outline: isRemovable isMovable 
abilities { move: true, remove: true }
 
Object { elementId: null, visible: true, data: "part_3", "$element": {…}, elementGroup: {…}, bindings: {}, disabled: false, wasDisabled: false, "$label": {…}, label: "BS-daten", … }
OutlineControlsWidget.js:148:6
    onOutlineChange OutlineControlsWidget.js:148
    onUpdateOutlineControlButtons ve.ui.MWTemplateDialog.js:632
    emit oojs.js:901
    selectPartByPageName ve.ui.MWTransclusionOutlineContainerWidget.js:180
    onBookletLayoutSetPage ve.ui.MWTransclusionDialog.js:221
    emit oojs.js:901
    setPage BookletLayout.js:580
    onOutlineSelectWidgetSelect BookletLayout.js:266
    emit oojs.js:901
    selectItem SelectWidget.js:821
    selectItemByData SelectWidget.js:758
    focusPart ve.ui.MWTemplateDialog.js:649
    emit oojs.js:901
    emit oojs.js:901
    onTemplateParameterChoose ve.ui.MWTransclusionOutlineTemplateWidget.js:199
    emit oojs.js:901
    chooseItem SelectWidget.js:880
    onDocumentMouseUp SelectWidget.js:262

The problem stems from our use of "select" vs "highlight" semantics, we need to differentiate between the case where a template heading ("part") is selected, and when a parameter ("subitem") is selected, but rendered as highlighting and its containing template rendered as selected.

I might split some of the part-selection code to introduce a "highlight part" event, that styles the part without scrolling, focus, or affecting the outline buttons. Not sure if this is a wise name to give the new event because this is already a state we're abusing to highlight items—problematic itself because mouse hover interacts badly with highlighting.

Change 724020 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/VisualEditor@master] Distinguish between template select and \"highlight\"

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

awight moved this task from Doing to Review on the WMDE-TechWish-Sprint-2021-09-15 board.

Pulling this discussion out of the task description,

When a parameter is selected, the up/down and delete buttons are disabled.

Uh, no. Why?

@ECohen_WMDE Please check this assumption for us and link to or define what the toolbar buttons should look like when a parameter is selected. Thanks!

From Mattermost, for reference and to be considered by @ECohen_WMDE:

The misbehavior […] is "only" because the old sidebar is not yet disconnected. The old sidebar is flat. There is no way it can select a template and a parameter in the template the same time. The new sidebar was designed from the ground up to allow this. The problem is that the old sidebar still dictates how the new sidebar behaves.
Sure, we could "undo" our efforts, so to speak, and make the new sidebar as flat as the old one. That's pretty much what the suggestions implies. I'm not happy with this idea. Not only because it goes in the opposite direction of what we tried to do the past weeks. I also don't believe it will save us time in the long run. The moment we actually disconnect the old sidebar we will run into issues again, one way or the other.
I believe it should be possible to identify and disconnect the bad event that makes the buttons misbehave.

Pending design review. In the meeting today, we also noted that this feature interacts with:

  • Clicking on a template header causes the first parameter to be focused.
  • An unsaved changes warning would prevent accidentally deleting an entire template.

Clicking on a template header causes the first parameter to be focused.

  • It's intentional.
  • It's done to make it behave the same for all 3 types: wikitext parts focus the input field, template placeholders focus the search field. And templates focus the first input field as well.
  • Without this the behavior is partly unspecified/bogus as it depends on the presence of "something" (the definition of "something" being hardcoded in OO.ui.findFocusable) that can be focused in the ve.ui.MWTemplatePage that represents a template. That's not always the case, most notably when all the header says is "This template doesn't exist". And even if there is a link, focusing it is of no use for the majority of (mouse) users.
  • The patch that implemented this behavior was https://gerrit.wikimedia.org/r/719997.

An unsaved changes warning would prevent accidentally deleting an entire template.

I like that. 👍 I believe we have all the code that is necessary to do this.