Page MenuHomePhabricator

Link inspector appears when arriving in VE via the sticky header
Closed, ResolvedPublic

Description

Behavior

  1. Visit https://patchdemo.wmflabs.org/wikis/557db5804f/wiki/Douglas_Adams
  2. Log in (User: Patch Demo; PW: patchdemo1)
  3. Scroll the page such that the line at the top of the browser's viewport contains a link that spans two lines
  4. Click the "✏️" within the sticky header

Actual

  1. ❗️ VE opens with the link dialog open despite never having clicked or explicitly engaged with the link to which the dialog is related

Expected

  1. ✅ VE opens

Event Timeline

ppelberg edited projects, added VisualEditor; removed VisualEditor-Edit Cards .
ppelberg moved this task from To Triage to Triaged on the VisualEditor board.

I’m ambivalent about whether this should change. There’s a consistency to the current behavior that I appreciate. 🤔

There’s a consistency to the current behavior that I appreciate. 🤔

@DLynch can you describe the "consistency" you're referencing above? Are you referring to the fact that you appreciate that whenever a link is placed on/within a link, the link dialog shows?

Indeed. Whenever the caret is inside annotated text, we see the appropriate inspector.

The alternative involves us needing to have a whole state model for "how did we get here?" -- which we could have, but seems like added complexity for minimal benefit.

Another approach would be to always put the cursor at the start of a paragraph (the first paragraph that starts in the viewport). This means the cursor could end up half way down the page.

That does avoid my consistency complaint! It increases the odds of some edge-cases where we have to scroll in order to reach the first visible paragraph-start -- though probably very rare outside of small-screens / zoomed.

Implementation would look something like this:

  1. After finding where the cursor is, we know we are at an isContentOffset offset, i.e. content can be inserted here.
  2. If the content to the left is the opening of a CBN then we got lucky and we are done.
  3. If not, move to the last offset in the CBN, then use selectRelativeSelectableContentOffset to advance the cursor. The next offset should be at the start of a CBN.
  4. Check your new offset is still within the viewport. If not fall back to the original offset? (This describes the case of a very long paragraph filling the whole page)

I'll note that this also happens when the link doesn't really spans two lines, it just appears at the beginning or end of a line, but it's cartouche (the blue highlight) spans two lines.

image.png (2×3 px, 440 KB)

We have a separate bug about that (T111672), and I think if we fixed that, it would resolve this problem in the vast majority of cases.

Change 755704 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Prefer the start of a CBN when selectin first visible offset

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

Change 755704 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Prefer the start of a CBN when selecting first visible offset

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

Change 762538 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (71a2d651c)

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

Change 762538 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (71a2d651c)

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

Looks good to me. VE opens as expected.

ppelberg claimed this task.