Screenshot
Steps to reproduce:
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:
Example page 1: The first image on:
http://en.wikipedia.beta.wmflabs.org/w/index.php?title=User:RYasmeen_%28WMF%29&veaction=edit
Example Page 2: The first image on:
http://en.wikipedia.beta.wmflabs.org/w/index.php?title=20thmaychrome&veaction=edit
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()
(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.