Closed Bug 1634687 Opened 4 years ago Closed 4 years ago

Rename some address input event handlers

Categories

(Thunderbird :: Message Compose Window, task)

task

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: thomas8, Assigned: thomas8)

References

Details

Attachments

(1 file, 1 obsolete file)

Coming from bug 1629364, it was a bit strange to add address input sanitation to a blur event handler function called resetAddressContainer(element), which now contains the following:

  • 1 line to reset focus styling on the container (as function name suggests)
  • 26 lines doing input blur stuff like creating pills and input whitespace sanitation (not matching the function name)

Note that the addressContainer is not the element on which the blur event actually occured, so the function name doesn't refer to the target of the event which it handles in any way.

So I figured we could rename some of these address input event handlers to make their actual purpose more obvious, using a common pattern.

Current Name Proposed Name (generic pattern)
resetAddressContainer() --> addressInputOnBlur()
highlightAddressContainer() --> addressInputOnFocus()
recipientOnBeforeKeyDown() --> addressInputOnBeforeKeyDown()

Benefits of proposed naming pattern:

  • more generic
  • function name will still be appropriate even if we change/add content
  • easier to understand/identify in code
  • more informative/truthful about their purpose (because of including "addressInput" element reference and event name which delimits the context/use)
  • less prone to hijacking for alien purposes due to misleading function name

Function comments updated and polished accordingly.

This renames the address input event handling functions as explained in comment 0.

Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #9145015 - Flags: review?(alessandro)
Comment on attachment 9145015 [details] [diff] [review]
1634687_renameAddressInputEventHandlers.diff

Review of attachment 9145015 [details] [diff] [review]:
-----------------------------------------------------------------

For these architectural and code styling decisions is better to ping Magnus.
Attachment #9145015 - Flags: review?(alessandro) → review?(mkmelin+mozilla)
Comment on attachment 9145015 [details] [diff] [review]
1634687_renameAddressInputEventHandlers.diff

Review of attachment 9145015 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWidgets.js
@@ +2217,3 @@
>        });
>        input.onBeforeHandleKeyDown = event => {
> +        addressInputOnBeforeKeyDown(event, input);

For this one, wouldn't it be more appropriate to name it addressInputOnBeforeHandleKeyDown?

But why are we passing in the input? I think you could always use event.target and drop the element parameter
Attachment #9145015 - Flags: review?(mkmelin+mozilla)

updated patch per review of comment 3

Attachment #9145015 - Attachment is obsolete: true
Attachment #9145842 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9145842 [details] [diff] [review]
1634687_renameAddressInputEventHandlers.diff

Review of attachment 9145842 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin
Attachment #9145842 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 78.0

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/74fa74c3ca0a
Rename some address input event handlers. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Magnus, as you have approved these changes, is it ok with you if we expect incoming code to use this event handler naming pattern consistently? For all the reasons of comment 0, consistency, and a clear separation of layout and code with predictable, generic hooks.

Note that best practices (e.g. React) are recommending similar naming pattern:
https://jaketrent.com/post/naming-event-handlers-react/
https://medium.com/javascript-in-plain-english/handy-naming-conventions-for-event-handler-functions-props-in-react-fc1cbb791364

<tag id="someid"
     onclick="someidOnClick(event);"
     onkeydown="someidOnKeyDown(event);"/>
     
function someidOnClick(event) {
  helperfunction(...)  [if needed...]
}

function someidOnKeyDown(event) {
  checks and ifs and buts...
    do some stuff
  helperfunction(...)  [if needed...]
}
 
helperfunction(...) [called from elsewhere]

(In reply to Thomas D. from comment #0)

Current Name Proposed Name (generic pattern)
resetAddressContainer() --> addressInputOnBlur()
highlightAddressContainer() --> addressInputOnFocus()
recipientOnBeforeKeyDown() --> addressInputOnBeforeKeyDown()

Benefits of proposed naming pattern:

  • more generic
  • function name will still be appropriate even if we change/add content
  • easier to understand/identify in code
  • more informative/truthful about their purpose (because of including "addressInput" element reference and event name which delimits the context/use)
  • less prone to hijacking for alien purposes due to misleading function name
Flags: needinfo?(mkmelin+mozilla)

Sure.

Flags: needinfo?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #8)

Sure.

Thanks!

Another coding style question:

If we have a choice between...

  • creating dozens of hard-coded, complex event handling callers in code, to pass a specific associated element of each event target into a shared function called by event handlers (click and keydown), whereby keydown does not need this argument in 90% of cases; VS
  • keeping all callers simple and retrieve the associated element inside the shared function, from a dedicated attribute of each event target, where the invariable layout association between target element and associated element is expressed in the layout;

... which one is preferable?

This is similar to the case of passing this argument with callers, and you said it's better to retrieve the element from within the event handler function via event.target and keep callers simple (and I have seen that you are right...).

Complex callers with extra associatedElement argument passed around all over

function onload(elementObject) {
  targetElement.addEventListener("click", event => {
    let associatedElementID = elementObject.associatedElementID;
    targetOnClick(event, elementObject.associatedElementID);
  });
  targetElement.addEventListener("keydown", event => {
    let associatedElementID = elementObject.associatedElementID;
    targetOnKeydown(event, elementObject.associatedElementID);
  });

}

function targetOnClick(event, associatedElementID) {
  sharedFunction(event, associatedElementID)
}

function targetOnKeyDown(event, associatedElementID) {
  if (event.key == "XYZ") {
    // 90% of cases: associatedElementID not needed
  }
  if (event.key=="Enter") {
    // 10% of cases: 
    sharedFunction(event, associatedElementID)
  }
}
 
sharedFunction(event, associatedElementID) {
  >> act on event target and associated element
}

VS

Simple callers, then retrieve associatedElement from layout, inside the final shared function where it's needed

function onload(elementObject) {
  targetElement.addEventListener("click", event => {
    targetOnClick(event);
  });
  targetElement.addEventListener("keydown", event => {
    targetOnKeydown(event);
  });

}

function targetOnClick(event) {
  sharedFunction(event)
}

function targetOnKeyDown(event) {
  if (event.key == "XYZ") {
    ...
  }
  if (event.key=="Enter") {
    sharedFunction(event)
  }
}
 
sharedFunction(event) {
  let associatedElementID = event.target.dataset.associatedElementID;
  >> act on event.target and associated element
}

Flags: needinfo?(mkmelin+mozilla)

I think the second version is much preferable and easier to read.

Flags: needinfo?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #10)

I think the second version is much preferable and easier to read.

Thank you! Agreed! :-)

Severity: -- → N/A
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
You need to log in before you can comment on or make changes to this bug.