Last modified: 2014-05-29 19:38:29 UTC
Created attachment 15449 [details] 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
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() 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
