Review of attachment 9051492 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, this should now be simple to review. Now with shorter annotations. For details, see comment 35 ff. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress TB call stack (before this patch): (1) awDocumentKeyPress: if (addressCol1) > (2) awMenulistKeyPress: if (TAB) > (3) awTabFromMenulist: if (Shift+TAB): listBox.listBoxObject.ensureIndexIsVisible Which does nothing. So the entire call stack can be removed for TB. ::: 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&=comm-central No callers - no use. @@ -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; no-op: top.doNotCreateANewRow is only set to false in awInputChanged(), which is never called. With or without this, we never create a new row when pressing TAB on recipient input, which seems ok. @@ -738,5 @@ > - var row = awGetRowByInputElement(element); > - if (!event.shiftKey && row < top.MAX_RECIPIENTS) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1); no-op: this was used for ensureIndexIsVisible upon Shift+TAB, which is no longer needed, as changing focus auto-ensures visibility. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); So this is the lonely only line which remains, which could probably be realized in a more generic way, maybe onblur. @@ -746,5 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); > } > > -function awTabFromMenulist(element, event) { no-op: This is number (3) of the dispensable TB callstack (see above, MsgComposeCommands.js). ensureIndexIsVisible has no effect, already inbuilt with focus. > TB call stack (before this patch): > > (1) awDocumentKeyPress: if (addressCol1) > > (2) awMenulistKeyPress: if (TAB) > > (3) awTabFromMenulist: if (Shift+TAB): > listBox.listBoxObject.ensureIndexIsVisible @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { no-op: Number (2) of the dispensable TB stack. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { no-op: Number (1) of the dispensable TB callstack. SM has its own (only for mailing lists), with different functionality, so we preserve their listener which calls that in shared abMailListDialog.js. ::: mailnews/addrbook/content/abMailListDialog.js @@ +173,5 @@ > > AppendNewRowAndSetFocus(); > awFitDummyRows(1); > > +#ifdef MOZ_SUITE SM is still using this. No-op and erroneous for TB: this triggers the dispensable callstack involving awMenulistKeyPress, but recipient type selector menulist doesn't even exist in the mailing list dialog. That's because addressCol1 is the recipient type column in composition, but the recipient column in the mailing list dialog. Error-prone design, now removed. @@ +257,5 @@ > // completely. > document.getElementById("addressingWidget").disabled = true; > } > > +#ifdef MOZ_SUITE SM is still using this, but no-op for TB.
Bug 1528840 Comment 39 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 9051492 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, this should now be simple to review. Now with shorter annotations. For details, see comment 35 ff. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress TB call stack (before this patch): (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) > (2) [awMenulistKeyPress](https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress): if (TAB) > (3) [awTabFromMenulist]((https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist): if (Shift+TAB): listBox.listBoxObject.ensureIndexIsVisible Which does nothing. So the entire call stack can be removed for TB. ::: 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&=comm-central No callers - no use. @@ -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=awTabFromRecipient no-op: top.doNotCreateANewRow is only set to false in awInputChanged(), which is never called. With or without this, we never create a new row when pressing TAB on recipient input, which seems ok. @@ -738,5 @@ > - var row = awGetRowByInputElement(element); > - if (!event.shiftKey && row < top.MAX_RECIPIENTS) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1); no-op: this was used for ensureIndexIsVisible upon Shift+TAB, which is no longer needed, as changing focus auto-ensures visibility. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); So this is the lonely only line which remains, which could probably be realized in a more generic way, maybe onblur. @@ -746,5 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); > } > > -function awTabFromMenulist(element, event) { https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist no-op: This is number (3) of the dispensable TB callstack (see above, MsgComposeCommands.js). ensureIndexIsVisible has no effect, already inbuilt with focus. > TB call stack (before this patch): > > (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) > > (2) awMenulistKeyPress: if (TAB) > > (3) awTabFromMenulist: if (Shift+TAB): > listBox.listBoxObject.ensureIndexIsVisible @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress no-op: Number (2) of the dispensable TB stack. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress no-op: Number (1) of the dispensable TB callstack. SM has its own (only for mailing lists), with different functionality, so we preserve their listener which calls that in shared abMailListDialog.js. ::: mailnews/addrbook/content/abMailListDialog.js @@ +173,5 @@ > > AppendNewRowAndSetFocus(); > awFitDummyRows(1); > > +#ifdef MOZ_SUITE SM is still using this. No-op and erroneous for TB: this triggers the dispensable callstack involving awMenulistKeyPress, but recipient type selector menulist doesn't even exist in the mailing list dialog. That's because addressCol1 is the recipient type column in composition, but the recipient column in the mailing list dialog. Error-prone design, now removed. @@ +257,5 @@ > // completely. > document.getElementById("addressingWidget").disabled = true; > } > > +#ifdef MOZ_SUITE SM is still using this, but no-op for TB.
Review of attachment 9051492 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, this should now be simple to review. Now with shorter annotations. For details, see comment 35 ff. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress TB call stack (before this patch): (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) > (2) [awMenulistKeyPress](https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress): if (TAB) > (3) [awTabFromMenulist](https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist): if (Shift+TAB): listBox.listBoxObject.ensureIndexIsVisible Which does nothing. So the entire call stack can be removed for TB. ::: 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&=comm-central No callers - no use. @@ -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=awTabFromRecipient no-op: top.doNotCreateANewRow is only set to false in awInputChanged(), which is never called. With or without this, we never create a new row when pressing TAB on recipient input, which seems ok. @@ -738,5 @@ > - var row = awGetRowByInputElement(element); > - if (!event.shiftKey && row < top.MAX_RECIPIENTS) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1); no-op: this was used for ensureIndexIsVisible upon Shift+TAB, which is no longer needed, as changing focus auto-ensures visibility. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); So this is the lonely only line which remains, which could probably be realized in a more generic way, maybe onblur. @@ -746,5 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); > } > > -function awTabFromMenulist(element, event) { https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist no-op: This is number (3) of the dispensable TB callstack (see above, MsgComposeCommands.js). ensureIndexIsVisible has no effect, already inbuilt with focus. > TB call stack (before this patch): > > (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) > > (2) awMenulistKeyPress: if (TAB) > > (3) awTabFromMenulist: if (Shift+TAB): > listBox.listBoxObject.ensureIndexIsVisible @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress no-op: Number (2) of the dispensable TB stack. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress no-op: Number (1) of the dispensable TB callstack. SM has its own (only for mailing lists), with different functionality, so we preserve their listener which calls that in shared abMailListDialog.js. ::: mailnews/addrbook/content/abMailListDialog.js @@ +173,5 @@ > > AppendNewRowAndSetFocus(); > awFitDummyRows(1); > > +#ifdef MOZ_SUITE SM is still using this. No-op and erroneous for TB: this triggers the dispensable callstack involving awMenulistKeyPress, but recipient type selector menulist doesn't even exist in the mailing list dialog. That's because addressCol1 is the recipient type column in composition, but the recipient column in the mailing list dialog. Error-prone design, now removed. @@ +257,5 @@ > // completely. > document.getElementById("addressingWidget").disabled = true; > } > > +#ifdef MOZ_SUITE SM is still using this, but no-op for TB.
Review of attachment 9051492 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, this should now be simple to review. Now with shorter annotations. For details, see comment 35 ff. ::: mail/components/compose/content/MsgComposeCommands.js @@ -2567,5 @@ > dictionaryRemovalObserver.addObserver(); > > var identityList = document.getElementById("msgIdentity"); > > - document.addEventListener("keypress", awDocumentKeyPress, true); https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress TB call stack (before this patch): (1) awDocumentKeyPress: if (addressCol1) > (2) awMenulistKeyPress: if (TAB) > (3) awTabFromMenulist: if (Shift+TAB): listBox.listBoxObject.ensureIndexIsVisible Which does nothing. So the entire call stack can be removed for TB. (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress) (2) [awMenulistKeyPress](https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress) (3) [awTabFromMenulist](https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist) ::: 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&=comm-central No callers - no use. @@ -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=awTabFromRecipient no-op: top.doNotCreateANewRow is only set to false in awInputChanged(), which is never called. With or without this, we never create a new row when pressing TAB on recipient input, which seems ok. @@ -738,5 @@ > - var row = awGetRowByInputElement(element); > - if (!event.shiftKey && row < top.MAX_RECIPIENTS) { > - var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based. > - var listBox = document.getElementById("addressingWidget"); > - listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1); no-op: this was used for ensureIndexIsVisible upon Shift+TAB, which is no longer needed, as changing focus auto-ensures visibility. @@ -746,2 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); So this is the lonely only line which remains, which could probably be realized in a more generic way, maybe onblur. @@ -746,5 @@ > // when the user tabs out of an autocomplete line... > addRecipientsToIgnoreList(element.value); > } > > -function awTabFromMenulist(element, event) { https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist no-op: This is number (3) of the dispensable TB callstack (see above, MsgComposeCommands.js). ensureIndexIsVisible has no effect, already inbuilt with focus. > TB call stack (before this patch): > > (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) > > (2) awMenulistKeyPress: if (TAB) > > (3) awTabFromMenulist: if (Shift+TAB): > listBox.listBoxObject.ensureIndexIsVisible @@ -898,5 @@ > break; > } > } > > -function awMenulistKeyPress(event, element) { https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress no-op: Number (2) of the dispensable TB stack. @@ -1008,5 @@ > function awSizerMouseUp() { > document.removeEventListener("mousemove", awSizerMouseMove, true); > } > > -function awDocumentKeyPress(event) { https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress no-op: Number (1) of the dispensable TB callstack. SM has its own (only for mailing lists), with different functionality, so we preserve their listener which calls that in shared abMailListDialog.js. ::: mailnews/addrbook/content/abMailListDialog.js @@ +173,5 @@ > > AppendNewRowAndSetFocus(); > awFitDummyRows(1); > > +#ifdef MOZ_SUITE SM is still using this. No-op and erroneous for TB: this triggers the dispensable callstack involving awMenulistKeyPress, but recipient type selector menulist doesn't even exist in the mailing list dialog. That's because addressCol1 is the recipient type column in composition, but the recipient column in the mailing list dialog. Error-prone design, now removed. @@ +257,5 @@ > // completely. > document.getElementById("addressingWidget").disabled = true; > } > > +#ifdef MOZ_SUITE SM is still using this, but no-op for TB.