Bugzilla

(In reply to Marco Bonardo [:mak] from comment #3)
> This is a non-trivial change with associated risks. Is it part of a planned frontend project, or did you decide to work on this on your own?

Thanks for the feedback. Someone just asked on my gh page if I am willing to make a user script for them to the tune of bug 1673588. I did some research and got the impression from your posts in the other bugs that there was already general agreement, and specific details could be worked out in review, as with the older patch. I just made a new bug because I didn't wanna hijack someone else's, and I thought this would likely involve multiple bugs, so a task was more suitable.

> Taking small incremental fixes is easy, taking large rewrites of user interaction is not. The risk is you doing a lot of work and nobody able to pick it up in a short time frame.

I guess I didn't really see this as a large rewrite. It involves a bit of redundancy because the same behavior was already duplicated in several places, so I had to update it everywhere. I considered turning _whereToOpen into a shared helper function, but I didn't want to clutter the patch with unnecessary or tangential changes.

Well if you test the patch let me know what you think. I can separate the popup change and pref change into 2 patches now. It was a lot easier to work on them at the same time because it's tedious to test 1 without the other. I don't think this is really a major new feature compared to the other patches I worked on, since all the keyboard shortcuts already existed. I just tied them to preferences in the same way as the existing bookmarks prefs.

> This is an apparently easy problem, but very complex in practice when keeping into account all the possible combinations, coherency with the other UI views, and will of keeping things simple

I didn't think this was an easy problem - I did spend time tracking down all the possible permutations of prefs and shortcuts before writing a patch. Consistency with the other UI views was my main reason for doing it this way (e.g., see the bookmarks prefs).

> (not 100 different prefs generating infinite combinations to test).

In this case I added 2 new prefs, and only because there are already 2 existing `openintab` prefs. If it wasn't that way from the start I would have made a single `openintab` pref and a single `openinbackground` pref for both urlbar and searchbar. And consolidating them into 1 might be worthwhile, but rolling that into this patch would only make it even bigger.

> There's a lot of things to go through, including bug 1536756.

Bug 1536756 doesn't seem like a blocking issue, since this patch doesn't change the keyboard shortcuts themselves, it just adds a new pref that reverses tab and tabshifted and holds popups open in one condition. Bug 1753878 would change the actual key combinations so it would need to be coordinated with any changes to action override feature. But the key combos are the same in this patch, and I think the refactoring of the _whereToOpen methods in this patch will actually make resolving those bugs easier.

> I totally respect this will of contributing, but before undertaking such a large rewrite, you should plan with the frontend team and agree upon how to do it. Otherwise the risk is that it will stay in a limbo for a long time, because people may not have enough time to review and ensure this is safe enough for landing. That is pretty much the problem with Bug 1673588.

That's fine with me, I'm happy to take any and all feedback and collaborate with anyone. That is my intention in posting this bug. I was going to make a patch anyway so that I could test these features and get a sense of the experience for myself. At least for me, it's easier if it's not all in my imagination.

> Most new features should also be developed behind a pref, so that we ca neasily switch Release to the previous behavior, in case severe regressions are found.

One of the 2 changes is the addition of new prefs, so it didn't make sense to me to lock the new prefs behind an experimental pref. However, I guess that's feasible. The popup changes could also go behind a pref, but those are such minor changes. In any case, I don't think it's a good reason to go back to the drawing board.

> I think this requires a complete analysis and breakdown to try to tackle problems one at a time and reduce the risks.
> Mixing up multiple behavioral changes and re-architecture into a single bug is unlikely going to work if we want to mitigate risks and complexity.

I'm just trying to help move things forward. If there's a specific path forward you want to take, I'll certainly follow. I don't know of any other bugs that the popup change really depends on. The patch has solid test coverage. Breaking it up into smaller pieces would be a practical way to simplify this.

(In reply to Marco Bonardo [:mak] from comment #3)
> This is a non-trivial change with associated risks. Is it part of a planned frontend project, or did you decide to work on this on your own?

Thanks for the feedback. I certainly didn't intend to give the impression that I thought this was a trivial change without any risks. I have been trying to track down every implication of the current usability problems and every implication of the proposed changes. As for my motivation, someone just asked on my gh page if I am willing to make a user script for them to the tune of bug 1673588.

I did some research and got the impression from your posts in the other bugs that there was already general agreement, and specific details could be worked out in review, as with the older patch. I just made a new bug because I didn't wanna hijack someone else's, and I thought this would likely involve multiple bugs, so a task was more suitable.

> Taking small incremental fixes is easy, taking large rewrites of user interaction is not. The risk is you doing a lot of work and nobody able to pick it up in a short time frame.

I guess I didn't really see this as a large rewrite. It involves a bit of redundancy because the same behavior was already duplicated in several places, so I had to update it everywhere. I considered turning _whereToOpen into a shared helper function, but I didn't want to clutter the patch with unnecessary or tangential changes.

Well if you test the patch let me know what you think. I can separate the popup change and pref change into 2 patches now. It was a lot easier to work on them at the same time because it's tedious to test 1 without the other. I don't think this is really a major new feature compared to the other patches I worked on, since all the keyboard shortcuts already existed. I just tied them to preferences in the same way as the existing bookmarks prefs.

> This is an apparently easy problem, but very complex in practice when keeping into account all the possible combinations, coherency with the other UI views, and will of keeping things simple

I didn't think this was an easy problem - I did spend time tracking down all the possible permutations of prefs and shortcuts before writing a patch. Consistency with the other UI views was my main reason for doing it this way (e.g., see the bookmarks prefs).

> (not 100 different prefs generating infinite combinations to test).

In this case I added 2 new prefs, and only because there are already 2 existing `openintab` prefs. If it wasn't that way from the start I would have made a single `openintab` pref and a single `openinbackground` pref for both urlbar and searchbar. And consolidating them into 1 might be worthwhile, but rolling that into this patch would only make it even bigger.

> There's a lot of things to go through, including bug 1536756.

Bug 1536756 doesn't seem like a blocking issue, since this patch doesn't change the keyboard shortcuts themselves, it just adds a new pref that reverses tab and tabshifted and holds popups open in one condition. Bug 1753878 would change the actual key combinations so it would need to be coordinated with any changes to action override feature. But the key combos are the same in this patch, and I think the refactoring of the _whereToOpen methods in this patch will actually make resolving those bugs easier.

> I totally respect this will of contributing, but before undertaking such a large rewrite, you should plan with the frontend team and agree upon how to do it. Otherwise the risk is that it will stay in a limbo for a long time, because people may not have enough time to review and ensure this is safe enough for landing. That is pretty much the problem with Bug 1673588.

That's fine with me, I'm happy to take any and all feedback and collaborate with anyone. That is my intention in posting this bug. I was going to make a patch anyway so that I could test these features and get a sense of the experience for myself. At least for me, it's easier if it's not all in my imagination.

> Most new features should also be developed behind a pref, so that we ca neasily switch Release to the previous behavior, in case severe regressions are found.

One of the 2 changes is the addition of new prefs, so it didn't make sense to me to lock the new prefs behind an experimental pref. However, I guess that's feasible. The popup changes could also go behind a pref, but those are such minor changes. In any case, I don't think it's a good reason to go back to the drawing board.

> I think this requires a complete analysis and breakdown to try to tackle problems one at a time and reduce the risks.
> Mixing up multiple behavioral changes and re-architecture into a single bug is unlikely going to work if we want to mitigate risks and complexity.

I'm just trying to help move things forward. If there's a specific path forward you want to take, I'll certainly follow. I don't know of any other bugs that the popup change really depends on. The patch has solid test coverage. Breaking it up into smaller pieces would be a practical way to simplify this.

(In reply to Marco Bonardo [:mak] from comment #3)
> This is a non-trivial change with associated risks. Is it part of a planned frontend project, or did you decide to work on this on your own?

Thanks for the feedback. I certainly didn't intend to give the impression that I thought this was a trivial change without any risks. I have been trying to track down every implication of the current usability problems and every implication of the proposed changes. As for my motivation, someone just asked on my gh page if I am willing to make a user script for them to the tune of bug 1673588.

I did some research and got the impression from your posts in the other bugs that there was already general agreement, and specific details could be worked out in review, as with the older patch. I just made a new bug because I didn't wanna hijack someone else's, and I thought this would likely involve multiple bugs, so a task was more suitable.

> Taking small incremental fixes is easy, taking large rewrites of user interaction is not. The risk is you doing a lot of work and nobody able to pick it up in a short time frame.

I guess I didn't really see this as a large rewrite. It involves a bit of redundancy because the same behavior was already duplicated in several places, so I had to update it everywhere. I considered turning _whereToOpen into a shared helper function, but I didn't want to clutter the patch with unnecessary or tangential changes.

Well if you test the patch let me know what you think. I can separate the popup change and pref change into 2 patches now. It was a lot easier to work on them at the same time because it's tedious to test 1 without the other. I don't think this is really a major new feature compared to the other patches I worked on, since all the keyboard shortcuts already existed. I just tied them to preferences in the same way as the existing bookmarks prefs.

> This is an apparently easy problem, but very complex in practice when keeping into account all the possible combinations, coherency with the other UI views, and will of keeping things simple

I didn't think this was an easy problem - I did spend time tracking down all the possible permutations of prefs and shortcuts before writing a patch. Consistency with the other UI views was my main reason for doing it this way (e.g., see the bookmarks prefs).

> (not 100 different prefs generating infinite combinations to test).

In this case I added 2 new prefs, and only because there are already 2 existing `openintab` prefs. If it wasn't that way from the start I would have made a single `openintab` pref and a single `openinbackground` pref for both urlbar and searchbar. And consolidating them into 1 might be worthwhile, but rolling that into this patch would only make it even bigger.

> There's a lot of things to go through, including bug 1536756.

Bug 1536756 doesn't seem like a blocking issue, since this patch doesn't change the keyboard shortcuts themselves, it just adds a new pref that reverses tab and tabshifted and holds popups open in one condition. Bug 1753878 would change the actual key combinations so it would need to be coordinated with any changes to action override feature. But the key combos are the same in this patch, and I think the refactoring of the _whereToOpen methods in this patch will actually make resolving those bugs easier.

> I totally respect this will of contributing, but before undertaking such a large rewrite, you should plan with the frontend team and agree upon how to do it. Otherwise the risk is that it will stay in a limbo for a long time, because people may not have enough time to review and ensure this is safe enough for landing. That is pretty much the problem with Bug 1673588.

That's fine with me, I'm happy to take any and all feedback and collaborate with anyone. That is my intention in posting this bug. I was going to make a patch anyway so that I could test these settings and get a sense of the experience for myself. At least for me, it's easier if it's not all in my imagination.

> Most new features should also be developed behind a pref, so that we ca neasily switch Release to the previous behavior, in case severe regressions are found.

One of the 2 changes is the addition of new prefs, so it didn't make sense to me to lock the new prefs behind an experimental pref. However, I guess that's feasible. The popup changes could also go behind a pref, but those are such minor changes. In any case, I don't think it's a good reason to go back to the drawing board.

> I think this requires a complete analysis and breakdown to try to tackle problems one at a time and reduce the risks.
> Mixing up multiple behavioral changes and re-architecture into a single bug is unlikely going to work if we want to mitigate risks and complexity.

I'm just trying to help move things forward. If there's a specific path forward you want to take, I'll certainly follow. I don't know of any other bugs that the popup change really depends on. The patch has solid test coverage. Breaking it up into smaller pieces would be a practical way to simplify this.