Page MenuHomePhabricator

Editing captions in Firefox is so unpredictable as to be useless
Closed, ResolvedPublic1 Estimated Story Points

Description

Using Firefox (55.0.3 (64 bit)) is impossible to edit image captions without opening the Image dialog. In Chrome the caption can be edited as a normal text, but in Firefox the cursor stays at the beginning of the caption.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Deskana set the point value for this task to 8.
Deskana added a subscriber: Deskana.

When I tested this, I could actually edit the caption, but it behaved so strangely it was mostly unusable. I could type at the start of the caption, and cursor past the existing one, but not edit inside it. I could delete the whole caption, but it'd add a bunch of spaces where it shouldn't and then break things up onto multiple lines. After enough fiddling around, it wouldn't even move the cursor any more.

Deskana renamed this task from Can't edit Captions in Firefox without opening Image dialog to Editing captions in Firefox is so unpredictable as to be useless.Sep 1 2017, 10:15 AM
Deskana triaged this task as Medium priority.
Deskana moved this task from To Triage to TR0: Interrupt on the VisualEditor board.

Interestingly it works fine in the standalone demo, so must just be some MW weirdness...

Looks like a browser bug, if we don't set -moz-user-select: none on ve-ce-focusableNode * and then reset it to text on ve-ce-activeNode it works fine. Strangely it works in standalone, where the CSS is very similar.

Looks like this is a regression caused by https://gerrit.wikimedia.org/r/#/c/366898/ (pinging @Arlolra), which changed figcaptions to be display:block inside display:table, instead of display:table-caption. We discussed the invalidness of this at the time, but it didn't appear to be problematic. Now it does...

If putting display:block inside display:table is going to trigger mystery browser bugs we should definitely avoid it. So I'd suggest we revisit that decision on the figure DOM structure, and attach the zoom icon back to the figcaption. IIRC the only downside to that was that the icon wouldn't show if the caption was empty, but that could either be ignored or worked around.

I haven't looked into this yet, but as a general first comment, that patch is about the border-bottom, not the magnify link.

The reason for not using table-caption, as before, is that it implies the styling for the border-bottom needs to go on the figcaption and is therefore not present when that's omitted, as the commit message says. A solution there would be to always emit a figcaption, even when empty.

Change 376864 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[VisualEditor/VisualEditor@master] Fix editing figcaptions

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

I don't think there's anything inherit in the figcaption being displayed as a block nested in a table that's preventing this from working. An isolated test case of just that works fine. I think it's the opposite, there's something special in it being displayed as a table-caption that gets it to work at all. My hunch is that because the table-caption is displayed outside the table, it isn't subject to some inherited property of some box above it. Firefox seems to have issues with inheritance of the -moz-user-select properties, judging from some comments in the VE source.

If you remove those -moz-user-selects entirely, both the none and text, it also works.

Anyways, above is a patch that gets it to work. Not sure how acceptable it is though. I can dig further next week.

Interesting, the moz documentation doesn't mention the prefixed value: https://developer.mozilla.org/en-US/docs/Web/CSS/user-select

MozText, // Like TEXT, except that it won't get overridden by ancestors having ALL.

from https://github.com/mozilla/gecko-dev/blob/738b66b34c4d1d0c7616332720e6915c29549c87/layout/style/nsStyleConsts.h#L200

So, that kind of points at the problem.

If block were really the issue, then switching to say, table-cell or table-row (something ostensibly more semantically correct) for the figcaption would resolve it, but it doesn't. Along the lines of the hunch above, the magic of table-caption is that it probably changes what's considered an ancestor.

A less hacky solution might be to look at where moz-user-select all is being set (grep tells me there're a bunch of places) and deduce from there.

Change 376864 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Fix editing figcaptions in Firefox

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

Change 377336 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (02a2ea954)

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

Change 377336 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (02a2ea954)

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

Deskana assigned this task to Arlolra.
Deskana changed the point value for this task from 8 to 1.

This seems to be still broken on test2 running wmf.18.

This seems to be still broken on test2 running wmf.18.

Do you have any cached resources? Testing on https://test2.wikipedia.org/wiki/Server?veaction=edit seemed fine.

This seems to be still broken on test2 running wmf.18.

Do you have any cached resources? Testing on https://test2.wikipedia.org/wiki/Server?veaction=edit seemed fine.

Okay found the steps, it happened when I tried to add a caption to an existing image. It is a bit difficult to reproduce at this moment because of T175943, but found an older revision of my user page with an image with no caption on Beta cluster and it was happening there too. So this particular case of editing empty caption field is happening for both Beta Cluster and test2.

Well, this appears to still be a problem, so I'm reopening.

Deskana raised the priority of this task from Medium to High.Sep 18 2017, 5:33 PM

it happened when I tried to add a caption to an existing image

I see, yes, exacerbated by the "fix" here. Okay, I'll look into it again.

Change 379613 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[VisualEditor/VisualEditor@master] Guard against empty nodes

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

The selection seems to be prevented by a throw.

With the patch from T175852 / T175943, this seems to be resolved.

With the patch from T175852 / T175943, this seems to be resolved.

Where is it resolved? I can still reproduce it on Beta cluster. That patch also seems is not merged yet.

I tested it locally. The VE core patch is merged, but the update to the submodule in the extension hasn't yet.

I tested it locally. The VE core patch is merged, but the update to the submodule in the extension hasn't yet.

ah I see :)