search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Need to call onClosed of base class to clean up #5637

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by redmunds Wednesday Dec 04, 2013 at 07:30 GMT Originally opened as https://github.com/adobe/brackets/pull/6176

This is for #6174.

This code overrides onClosed of base class so we can close dropdown, but wasn't calling the base class method.

cc@njx

redmunds included the following code: https://github.com/adobe/brackets/pull/6176/commits

core-ai-bot commented 3 years ago

Comment by njx Wednesday Dec 04, 2013 at 18:30 GMT

Reviewed.

core-ai-bot commented 3 years ago

Comment by redmunds Wednesday Dec 04, 2013 at 19:52 GMT

@njx Thanks, that's a better solution. Fixed.

Also, as you mentioned in the original PR, I had to prevent close button from stopping propagation of click event for this case:

  1. Open 2 CSS inline editors
  2. Show New Rule drop down list on second inline editor
  3. Click on close button of first inline editor

If close button stops propagation of close event, then dropdown list stays open, and second inline editor moves up (due to first inline editor closing), and dropdown is not repositioned (so it's in wrong place).

core-ai-bot commented 3 years ago

Comment by njx Wednesday Dec 04, 2013 at 23:27 GMT

This looks fine to me.@RaymondLim - I noticed this PR got assigned to you - do you want to finish reviewing/testing it, or do you want to reassign it to me?

core-ai-bot commented 3 years ago

Comment by RaymondLim Wednesday Dec 04, 2013 at 23:32 GMT

Reassign it to@njx

core-ai-bot commented 3 years ago

Comment by njx Wednesday Dec 04, 2013 at 23:36 GMT

Hmmm, actually, I'm seeing another problem now: if I just open an inline editor and then click the close button, focus doesn't go back to the main editor. Might be related to the removal of the stopPropagation?

core-ai-bot commented 3 years ago

Comment by njx Wednesday Dec 04, 2013 at 23:37 GMT

So that makes me think we should go back to the capture handler, but only close the dropdown there if the click target isn't inside the dropdown, as I mentioned in the other PR.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Dec 04, 2013 at 23:38 GMT

(and we'd have to restore the stopImmediatePropagation(), but that should be okay since in the cases we care about the capture handler will get first crack no matter what's clicked on)

core-ai-bot commented 3 years ago

Comment by redmunds Thursday Dec 05, 2013 at 00:59 GMT

@njx Hopefully this is the magic incantation :) I tested all of the cases hit so far, and ran all unit tests.

core-ai-bot commented 3 years ago

Comment by njx Thursday Dec 05, 2013 at 01:11 GMT

Yup, seems to be working fine to me. I just made a couple nit-picky comments -- feel free to merge once you've addressed them (or decide not to).

core-ai-bot commented 3 years ago

Comment by redmunds Thursday Dec 05, 2013 at 01:42 GMT

Fixed nits. Merging.