895429 - Change nooverflow attribute to overflows

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

"nooverflow" is a negative attribute, and it's also awkward with those two o's in there.

Mike de Boer suggestion changing this attribute to "overflows" (which defaults to true if not present).

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

To fix this bug, you'll need to clone the repository at https://hg.mozilla.org/projects/ux/.

Search for "nooverflow" in the /browser directory to see where it is referenced.

Comment 2

[Akshat Kedia [:aks]]

I can help here !

Comment 3

[Akshat Kedia [:aks]]

Attachment: 895429.patch

Hope this fixes stuff. :)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 795062 [details] [diff] [review]
895429.patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2280,5 @@
>  
>      while(child && this._target.clientWidth < this._target.scrollWidth) {
>        let prevChild = child.previousSibling;
>  
> +      if (!child.hasAttribute("overflows")) {

Please change this to:
if (child.getAttribute("overflows") != "false") {
since the overflows attribute is optional and the behavior should default to items being overflowed.

Comment 5

[Akshat Kedia [:aks]]

Attachment: 895429.patch

Made the required the changes. This is my first bug and the first patch I uploaded was when I attened a Mozilla BootCamp so there were people there to help me but now back home I am having a bit of problems on with the hg commands and the procedure. Hope this patch is works else please help me !

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

The new patch doesn't contain any differences from the previous patch.

If you are using patch queues, after you make your changes run `hg qref` to refresh the patch. Then upload the new patch.

If you are not using patch queues, then make sure that your changes are saved before running `hg diff > patch.txt`.

Comment 7

[mjh563]

Akshat, if you need any help with using hg or creating patches, you can always ask in #introduction on IRC (irc.mozilla.org).

It's a help channel for new contributors, and there are usually people there who can advise and answer questions. :)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Any update on this? Let me know if there is anything I can do to help.

Comment 9

[Akshat Kedia [:aks]]

Attachment: 895429.patch

Okay this time I did it completely on my own and finally learned how to make a patch ! :) Lets hope this patch is valid. If not please let me know where am I going wrong.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 808158 [details] [diff] [review]
895429.patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1629,5 @@
>          widget[prop] = aData[prop];
>        }
>      }
>  
> +    const kOptBoolProps = ["removable", "showInPrivateBrowsing", "overflows"]

nit: Please add a semicolon to the end of this line.

@@ +2370,3 @@
>          this._collapsed.set(child.id, this._target.clientWidth);
>          child.classList.add("overflowedItem");
> +        child.setAttribute("customizableui-anchorid", this._onClickChevronron.id);

This looks broken, this._onClickChevronron doesn't exist.

Comment 11

[Akshat Kedia [:aks]]

Attachment: 895429.patch

Made the above changes.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 809144 [details] [diff] [review]
895429.patch

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

Thanks! These changes look good, and they passed all the customizableui tests on my local machine. I'll push it to the UX branch now.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 809144 [details] [diff] [review]
895429.patch

http://hg.mozilla.org/projects/ux/annotate/0e19ff24a086/browser/base/content/browser.js#l2185 also needed to be updated to use 'overflows' but I fixed that for you.

Congratulations on fixing your first bug!

https://hg.mozilla.org/projects/ux/rev/07c558e72010

Comment 14

[Akshat Kedia [:aks]]

Woohoo ! :D Thanks a ton for being with me on this the whole time :)

Comment 15

[Akshat Kedia [:aks]]

Can you please vouch for my Mozillians profile. Here's the link https://mozillians.org/en-US/u/aksht.kedia/ Thank You !

Comment 16

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/07c558e72010