Page MenuHomePhabricator

VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears
Closed, ResolvedPublic

Description

Screenshot

Steps to reproduce:

  1. Insert a frameless-left aligned image

2.Save the page
3.Reopen the page
4.Open the Media Settings dialog for the image

Observed Result:
There is no text field for the caption,Cannot close the Media Settings dialog box.
The following error appears in the console:
TypeError: node.getCaptionNode is not a function

See the screenshot attached


Version: unspecified
Severity: normal

Attached:

Details

Reference
bz65568

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:10 AM
bzimport set Reference to bz65568.

This happens because imgModel.getImageNodeType() returns 'mwBlockImage', but the actual node is an inline image.

So 1) we shouldn't base decisions on what to do with the node object on the image model's opinion of what class the node should be, 2) that opinion should be used in very very few places, everywhere else we should be using node.getType() and 3) the image model's opinion should not be wrong :)

Most times, we can't trust the node.getType() because changes in the model changes the node's type -- that's what the model's test was for.

It should, however, not lie or make mistakes. This is going to be somewhat complex, since wikitext's way of transforming between inline/block can be extremely arbitrary-looking.

One more thing that we could do, however, is use node.getType() whenever that *can* be trusted. This means that on loading the node (like in this bug) the node's type can be used, and same with after inserting a new node.

In the other cases, though (like checking whether we're going to change a node type or just change the attributes) the node.getType() won't help us, we'll have to use a custom test inside the model to check.

(In reply to Moriel Schottlender from comment #3)

Most times, we can't trust the node.getType() because changes in the model
changes the node's type -- that's what the model's test was for.

Well, you can most certainly trust node.getType() to tell you if calling node.getCaptionNode() will cause it to explode or not, and you apparently can't trust imgModel.getImageNodeType() in this case ;)

It should, however, not lie or make mistakes. This is going to be somewhat
complex, since wikitext's way of transforming between inline/block can be
extremely arbitrary-looking.

Yeah, it should be improved, but if it makes mistakes the consequences shouldn't be /this/ bad.

One more thing that we could do, however, is use node.getType() whenever
that *can* be trusted. This means that on loading the node (like in this
bug) the node's type can be used, and same with after inserting a new node.

Yes, exactly.

In the other cases, though (like checking whether we're going to change a
node type or just change the attributes) the node.getType() won't help us,
we'll have to use a custom test inside the model to check.

Yup, that sounds reasonable. However, we'd also have to define what happens when we inspect a node whose type appears to be "wrong"; we probably shouldn't change its type if the user didn't change any of the flags. OTOH, this just shouldn't happen.

Change 134554 had a related patch set uploaded by Mooeypoo:
Fix MWImageModel's getImageNodeType()

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

Change 134554 merged by jenkins-bot:
Fix MWImageModel's getImageNodeType()

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

(In reply to Roan Kattouw from comment #4)

Well, you can most certainly trust node.getType() to tell you if calling
node.getCaptionNode() will cause it to explode or not, and you apparently
can't trust imgModel.getImageNodeType() in this case ;)

Yes, fallbacks should be included. I think the current (merged) commit dealt with that, I hope no more bugs will come with it, but I'll go over the code to verify.

Yup, that sounds reasonable. However, we'd also have to define what happens
when we inspect a node whose type appears to be "wrong"; we probably
shouldn't change its type if the user didn't change any of the flags. OTOH,
this just shouldn't happen.

That would be a problem, though -- how would we know that the type is wrong? It's not like the user consciously chooses inline vs block. The inline/block type is defined by seemingly arbitrary flags. They're not completely arbitrary, which is good, but they're also not entirely intuitive either. The only way to know for sure is through the tests in the getImageNodeType. It just has to be verified to follow the right conditions.

In this case, I had a typo in there, and from my tests so far this *should* be true for all the cases I tested, but I am slightly concerned we may be missing things with wikitext, or that if new types are added we will have to adjust as well.

Verified the fix in Betalabs

Verified the fix in test2

Verified the fix in production