Review of attachment 9049117 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, here's annotations to assist your review of this patch. Patch has one flaw pertaining to the spaghetti code with Seamonkey mix-ins, see comment 35. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); This ultimately ends up in ensureIndexIsVisible(), which doesn't do anything, so it can go away. This is TB only for composition. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ -547,5 @@ > - var lastInput = awGetInputElement(top.MAX_RECIPIENTS); > - if (lastInput && lastInput.value && !top.doNotCreateANewRow) > - awAppendNewRow(false); > - top.doNotCreateANewRow = false; > -} https://dxr.mozilla.org/comm-central/search?q=awInputChanged no callers: awInputChanged() is not used at all in TB, neither in SM. @@ -732,5 @@ > > function awTabFromRecipient(element, event) { > - // If we are the last element in the listbox, we don't want to create a new row. > - if (element == awGetInputElement(top.MAX_RECIPIENTS)) > - top.doNotCreateANewRow = true; https://dxr.mozilla.org/comm-central/search?q=doNotCreateANewRow top.doNotCreateANewRow is only reset to false in awInputChanged() which is never called, so that's not functional and can go away. Currently we never create new rows with tab, which seems ok. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); ToDo: In another bug, this should probably be tied to some other event, e.g. blur. We have no way of telling when the recipient address is ready or not; just clicking elsewhere is as good to exit recipient input as any other method. OT: I also wonder about the data/privacy of this: where does the spellcheck ignore list live? Is it temporary or permanent? Is it accessible to the user? Which part(s) of the recipient input are we adding there? Emails, too? Is this known to the user? @@ -751,5 @@ > - var row = awGetRowByInputElement(element); > - if (event.shiftKey && row > 1) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1); https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist awTabFromMenulist() is only used to ensure visibility of previous row for Shift+Tab from recipient type selector - no longer needed. only caller: in awMenulistKeyPress() below - dispensible, too. @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist This function currently only checks for tab key, then calls awTabFromMenulist(), which does nothing useful (see above), so let's eliminate. Btw, keypress event and keyCode are also deprecated. The only TB caller is function awDocumentKeyPress(), which does nothing else, so also dispensable in principle (see below; caveats: see comment 35). ToDo: Here or in another bug, we may want to capture other key presses on the menu list (using keydown), namely those that do NOT shift focus to the recipient input, and ensure that the input becomes visible again in case the user scrolled it away whilst it had focus. Edge case but relevant because currently you can change the recipient type without seeing it, which is error-prone. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress In TB, this function only calls awMenuListKeyPress(), which is useless (see above), so this could go too. However, the function is also called from mailing lists which is shared between TB and SM. In TB's mailing lists, it shouldn't even be called, that's a bug (no recipient type selector menulist in mailing lists). In SeaMonkey's mailing lists, it calls awRecipientKeyPress(), which handles cursor down/up and deletes for mailing lists. ToDo: Sooo, we can't remove awDocumentKeyPress() for TB without also removing TB's callers (which makes this patch incomplete); we can't just remove the mailing list caller as it's currently shared with SM; and we can't remove the SM caller as it's still in use afasics. (Although I wonder how SM avoids the composition address widget functions from applying in the mailing list, because in TB it seems shared functionality.) What we could do is to only keep the callers for SM at compile time, using #if. See comment 35 for details.
Bug 1528840 Comment 37 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9049117 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, here's annotations to assist your review of this patch. Patch has one flaw pertaining to the spaghetti code with Seamonkey mix-ins, see comment 35. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); This ultimately ends up in ensureIndexIsVisible(), which doesn't do anything, so it can go away. This is TB only for composition. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ -547,5 @@ > - var lastInput = awGetInputElement(top.MAX_RECIPIENTS); > - if (lastInput && lastInput.value && !top.doNotCreateANewRow) > - awAppendNewRow(false); > - top.doNotCreateANewRow = false; > -} https://dxr.mozilla.org/comm-central/search?q=awInputChanged no callers: awInputChanged() is not used at all in TB, neither in SM. @@ -732,5 @@ > > function awTabFromRecipient(element, event) { > - // If we are the last element in the listbox, we don't want to create a new row. > - if (element == awGetInputElement(top.MAX_RECIPIENTS)) > - top.doNotCreateANewRow = true; https://dxr.mozilla.org/comm-central/search?q=doNotCreateANewRow top.doNotCreateANewRow is only reset to false in awInputChanged() which is never called, so that's not functional and can go away. Currently we never create new rows with tab, which seems ok. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); ToDo: In another bug, this should probably be tied to some other event, e.g. blur. We have no way of telling when the recipient address is ready or not; just clicking elsewhere is as good to exit recipient input as any other method. OT: I also wonder about the data/privacy of this: where does the spellcheck ignore list live? Is it temporary or permanent? Is it accessible to the user? Which part(s) of the recipient input are we adding there? Emails, too? Is this known to the user? @@ -751,5 @@ > - var row = awGetRowByInputElement(element); > - if (event.shiftKey && row > 1) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1); https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist awTabFromMenulist() is only used to ensure visibility of previous row for Shift+Tab from recipient type selector - no longer needed. only caller: in awMenulistKeyPress() below - dispensible, too. @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist This function currently only checks for tab key, then calls awTabFromMenulist(), which does nothing useful (see above), so let's eliminate. Btw, keypress event and keyCode are also deprecated. The only TB caller is function awDocumentKeyPress(), which does nothing else, so also dispensable in principle (see below; caveats: see comment 35). ToDo: Here or in another bug, we may want to capture other key presses on the menu list (using keydown), namely those that do NOT shift focus to the recipient input, and ensure that the input becomes visible again in case the user scrolled it away whilst it had focus. Edge case but relevant because currently you can change the recipient type without seeing it, which is error-prone. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress In TB, this function only calls awMenuListKeyPress(), which is useless (see above), so this could go too. However, the function is also called from mailing lists which is shared between TB and SM. In TB's mailing lists, it shouldn't even be called, that's a bug (no recipient type selector menulist in mailing lists). In SeaMonkey's mailing lists, it calls awRecipientKeyPress(), which handles cursor down/up and deletes for mailing lists. ToDo: Sooo, we can't remove awDocumentKeyPress() for TB without also removing TB's callers (which makes this patch incomplete); we can't just remove the mailing list caller as it's currently shared with SM; and we can't remove the SM caller as it's still in use afasics. (Although I wonder how SM avoids the composition address widget functions from applying in the mailing list, because in TB it seems shared functionality.) What we could do is to only keep the callers for SM at compile time, using #ifdef MOZ_SUITE. See comment 35 for details.