The drop marker appears below BMB_bookmarksShowAll, at the end of the menupopup

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: chiragbhatia2006, Mentored)

Tracking

unspecified
Firefox 39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Looks like we forgot this attribute, that means it's possible to try to drag a bookmark below it (the drag indicator appears below it).

The scope of this bug is to add the attribute and check the drag indicator doesn't appear there anymore after having done that.
Assignee

Comment 1

4 years ago
I can take this bug as my first if you could describe how to reproduce it and direct me where the code change is required.
Flags: needinfo?(mak77)

Comment 2

4 years ago
Could I be assigned to this issue?
Reporter

Comment 3

4 years ago
Sorry, Chirag Bhatia posted first, so I'm going to assign this bug to him.

To reproduce the bug you can try to drag&drop a bookmark below the Show all bookmarks entry that is at the bottom of the bookmarks menu button (the dropdown at the right of the star in the navigation bar).
It will currently show a dropmarker there, while it shouldn't.
That element has id BMB_bookmarksShowAll, you can find it in browser.xul (you can search using http://mxr.mozilla.org/mozilla-central/ or https://dxr.mozilla.org/)
The element should grow a builder attribute with value "end". then the dropmarker should not appear anymore.

wrahman0, if you have some experience with cpp, you could help me with bug 1067054, otherwise also bug 901952 is worth it.
Flags: needinfo?(mak77)
Assignee

Comment 4

4 years ago
Posted image screenshot.jpg
Hi Marco, thanks for assigning it to me and helping me out with this.

That didn't seem to work.

Here's what I tried:
I edited this tag in browser.xul - https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#944 so that the code finally looked like this:

             <menuitem id="BMB_bookmarksShowAll"
                     class="subviewbutton panel-subview-footer"
                     label="&showAllBookmarks2.label;"
                     command="Browser:ShowAllBookmarks"
                     key="manBookmarkKb"
                     builder="end"/>

After compiling and running, I can still see the dropmarker (the little white arrow below the black cursor, as seen in the screenshot attached). Please let me know if I'm doing anything wrong or if I should be trying to solve this bug in another way.
Flags: needinfo?(mak77)
Assignee

Comment 5

4 years ago
Posted image screenshotafter.jpg
Ok, I realized I was checking the wrong 'Show All Bookmarks' entry in the previous screenshot, so here's an updated one (this screenshot was also taken after the changes made in the above comment). Just to clarify, I think there are multiple dropmarkers, the mouse cursor changing with a white arrow below it and the subtle blue line that comes below the 'Show All Bookmarks' entry. Do we need to remove both of them or just one of them?
Reporter

Comment 6

4 years ago
ugh, looks like the code changed with this changeset and I didn't recall: http://hg.mozilla.org/mozilla-central/diff/c175d9ca4939/browser/components/places/content/browserPlacesViews.js


So, builder=end doesn't seem to be useful in this case, cause the behavior is driven by the presence of "panel-subview-footer" class on the element.

The bug must be elsewhere, unfortunately it requires to be debugged yet (that is, I cannot give you line by line instructions until we figure out where the code fails). The code that hides the dropmarker should be this one
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#43
You can try to play with it and print out the values of the calculations done there in the browser console or in the shell using dump(). That could help us understand which of the values is returning something unexpected.
I suspect there is something wrong with _endMarker...
Flags: needinfo?(mak77)
Summary: add builder="end" attribute to BMB_bookmarksShowAll → The drop marker appears below BMB_bookmarksShowAll, at the end of the menupopup
Assignee

Comment 7

4 years ago
Posted image screenshot3.jpg
Hi Marco,

I tried playing around a bit with the browser.xul code and tried removing "panel-subview-footer" class and added the builder=end attribute to the element (as seen in code below), but it still didn't remove the dropmarker (as seen in screenshot).

<menuitem id="BMB_bookmarksShowAll"
        class="subviewbutton"
        label="&showAllBookmarks2.label;"
        command="Browser:ShowAllBookmarks"
        key="manBookmarkKb"
        builder="end"/>
Reporter

Comment 8

4 years ago
yeah I think the bug (whatever it is) is in menu.xml detection, rather than in the definition of the element.
Assignee

Comment 9

4 years ago
Ok,

I tried putting a few console.log() messages as well as dump() messages in this line http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#56 just before the return is called, but I'm unable to see any of my output either in the browser's JavaScript console or from the shell that I'm running (I'm running this on a Linux shell, if that matters).

Am I doing something wrong, or is the code not getting called as I expect it to?
Flags: needinfo?(mak77)
Reporter

Comment 10

4 years ago
You might try Services.console.logStringMessage().

dump should work and dump to the shell, remember you must add \n into your messages or they could just disappear in the middle of other warnings.
Something like dump("\nyour message\n\n"); should be quite visible.
Flags: needinfo?(mak77)
Assignee

Comment 11

4 years ago
Hi Marco,

I tried using Services.console.logStringMessage() as well as dump() with newline characters, but I'm still unable to see any log msgs in the shell or the browser console. I guess the code within that function is not getting called. I'm guessing dragging a bookmark over the menu pop up (or items within it) triggers the function call. Or am I missing something here?
Reporter

Comment 12

4 years ago
Are you in a debug build?
I modified the method like this

      <method name="_hideDropIndicator">
        <parameter name="aEvent"/>
        <body><![CDATA[
          let target = aEvent.target;

          // Don't draw the drop indicator outside of markers.
          // The markers are hidden, since otherwise sometimes popups acquire
          // scrollboxes on OS X, so we can't use them directly.
          let firstChildTop = this._startMarker.nextSibling.boxObject.y;
          let lastChildBottom = this._endMarker.previousSibling.boxObject.y +
                                this._endMarker.previousSibling.boxObject.height;
          let betweenMarkers = target.boxObject.y >= firstChildTop ||
                               target.boxObject.y <= lastChildBottom;
dump(`\n${target} - ${betweenMarkers}\n\n`);
          // Hide the dropmarker if current node is not a Places node.
          return !(target && target._placesNode && betweenMarkers);
        ]]></body>
      </method>

I recompiled browser (./mach build browser) and I can see the messages in my terminal when hovering (after ./mach run).
Looks like you might have some issues with building, or you're not running what you are building...

betweenMarkers is true at the end of the menupopup, it should be false, so the calc is somehow broken. I didn't figure out where's the brokeness there, but I think we can simplify all that boxObject madness by using the DOM util Node.compareDocumentPosition()
https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition

The other issue is that target is expected to always be a menuitem here, but at the very top and very bottom of the popup, due to recent styling changes, we are effectively dropping on the menupopup.
This could be easily fixed by adding at the beginning a:

// Don't show a drop indicator when dragging over the popup.
if (target.localName == "menupopup")
  return true;

But looks like this is not needed if we properly fix betweenMarkers.

So in the end I modified the method like this:

      <!-- Check if we should hide the drop indicator for the target -->
      <method name="_hideDropIndicator">
        <parameter name="aEvent"/>
        <body><![CDATA[
          let target = aEvent.target;
          // Don't draw the drop indicator outside of markers or if current
          // node is not a Places node.
          let betweenMarkers = (this._startMarker.compareDocumentPosition(target) & Node.DOCUMENT_POSITION_FOLLOWING) &&
                               (this._endMarker.compareDocumentPosition(target) & Node.DOCUMENT_POSITION_PRECEDING);
          return !(target && target._placesNode && betweenMarkers);
        ]]></body>
      </method>

And it seems to do the job. But I didn't test it very deeply, so it would be good if you could confirm the goodness of this solution and do some testing with it.
Flags: needinfo?(nobody)
Reporter

Updated

4 years ago
Assignee: nobody → chiragbhatia2006
Status: NEW → ASSIGNED
Flags: needinfo?(nobody) → needinfo?(chiragbhatia2006)
Assignee

Comment 13

4 years ago
Posted image Screenshot.jpg
Hi Marco,

I tried your code with the dump() method, but still couldn't get any dump msgs in my terminal.

For the record, I'm using Arch linux, bash terminal. I'm building using "python2 ./mach build" though I tried "python2 ./mach build browser" as well and it built much faster. Then I'm using "python2 ./mach run -P" to run the browser.

I thought about what you said about me not running what I'm building, so I tried changing the "Show All Bookmarks" string at /browser/locales/en-US/chrome/browser/browser.dtd and that change did reflect after I rebuilt and ran it again.

I then tried your 2nd code which should have solved the problem, but I still see the dropmarker.

Any ideas?
Flags: needinfo?(chiragbhatia2006) → needinfo?(mak77)
Reporter

Comment 14

4 years ago
there should be no need to explicitly invoke python. As well as you don't need -P (a new profile is generated automatically)

I'm not sure why you cannot see the changes to menu.xml, maybe you are reusing a profile that has a xul cache, try passing -purgecaches
Could you please attach your .mozconfig file? Just in case.
Flags: needinfo?(mak77)
Assignee

Comment 15

4 years ago
I was explicitly invoking python because Arch Linux has python 3 as the default one, and mach requires python2. But I think mozconfig should have taken care of it I guess.

I tried passing -purgecaches to ./mach run, but still no luck. Still seems like the code isn't being triggered.

Here's the contents of my .mozconfig:

mk_add_options PYTHON=/usr/bin/python2
mk_add_options AUTOCONF=autoconf-2.13
mk_add_options AUTOCLOBBER=1
Flags: needinfo?(mak77)
Reporter

Comment 16

4 years ago
Maybe try with a debug build
ac_add_options --disable-optimize
ac_add_options --enable-debug

I'm finishing up the ideas...
Flags: needinfo?(mak77)
Assignee

Comment 17

4 years ago
Hi Marco,

My builds have been failing since yesterday after I did an "hg pull -uv". I even tried "hg revert --all", but still I keep getting this error:

 0:10.64 /mnt/extgit/mozilla-central/obj-x86_64-unknown-linux-gnu/browser/components/build/tmp6Si0nj.list:
 0:10.64     INPUT("nsModule.o")
 0:10.64     INPUT("../about/AboutRedirector.o")
 0:10.64     INPUT("../dirprovider/DirectoryProvider.o")
 0:10.64     INPUT("../feeds/nsFeedSniffer.o")
 0:10.64     INPUT("../shell/nsGNOMEShellService.o")
 0:10.64     INPUT("../../../memory/fallible/fallible.o")
 0:10.64 
 0:10.64 ../../dist/include/js/HeapAPI.h:228: error: undefined reference to 'js::gc::AssertGCThingHasType(js::gc::Cell*, JSGCTraceKind)'
 0:10.64 collect2: error: ld returned 1 exit status
 0:10.65 /mnt/extgit/mozilla-central/config/rules.mk:812: recipe for target 'libbrowsercomps.so' failed
 0:10.65 make[3]: *** [libbrowsercomps.so] Error 1
 0:10.65 /mnt/extgit/mozilla-central/config/recurse.mk:128: recipe for target 'libs' failed
 0:10.65 make[2]: *** [libs] Error 2
 0:10.65 /mnt/extgit/mozilla-central/config/recurse.mk:128: recipe for target 'libs' failed
 0:10.65 make[1]: *** [libs] Error 2
 0:10.65 /mnt/extgit/mozilla-central/config/rules.mk:541: recipe for target 'default' failed
 0:10.65 make: *** [default] Error 2

Is this something specific to my system or are you getting this too? I searched for the error on Google but got no hits.
Flags: needinfo?(mak77)
Reporter

Comment 18

4 years ago
nope, my builds are not failing for now, try to pull again and check with hg status that you don't have unexpected modifications (you can hg update -C to revert back to the current head). Did you see any merge errors?
Try also a ./mach clobber before ./mach build
Are you building off mozilla-central?
You might even ask build system maintainers in the #build channel.
In the worst case, you might have to clone again from scratch (use an hg bundle in such case, to do that faster).
Flags: needinfo?(mak77)
Assignee

Comment 19

4 years ago
Ok, I think ./mach clobber and another pull fixed the build for me.

I tried adding the two lines that you mentioned above in my .mozconfig to make a debug build and then tried both the code variations above (the one which is supposed to show msgs in the terminal and one which is supposed to fix the dropmarker from showing up on the button), but both of them still didn't work.

Any ideas?
Flags: needinfo?(mak77)
Reporter

Comment 20

4 years ago
I have no more ideas.

I'd try at least to dump something from 
<handler event="dragover"><![CDATA[
at
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#379

and see if at least this works, so we can figure if it's Linux not going through the expected path, or a real problem with the build. If you can see the dump, then you can add more dumps to figure why we don't enter the

if (dropPoint.folderElt || this._hideDropIndicator(event)) {
Flags: needinfo?(mak77)
Assignee

Comment 21

4 years ago
Hi Marco,

I deleted the entire local repository, and downloaded it again from mozilla central, rebuilt it and all changes started working :)

I can see the dump messages and your change above in menu.xml in comment#12 made the drop marker disappear.
Flags: needinfo?(mak77)
Reporter

Comment 22

4 years ago
good, looks like it was some fancy repository corruption in the end.

you can restart from comment 12, it should have most needed info to proceed.
Flags: needinfo?(mak77)
Assignee

Comment 23

4 years ago
Posted patch bugfix.patch (obsolete) — Splinter Review
The change you gave above Comment#12 worked fine on my system. The drop marker is no longer visible on the "Show All Bookmarks" entry.

This is the first time I'm creating a patch in mercurial, so let me know if there's anything wrong with it.
Flags: needinfo?(mak77)
Reporter

Comment 24

4 years ago
Comment on attachment 8581723 [details] [diff] [review]
bugfix.patch

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

So, the first thing to fix, is that you must add a Commit message to the patch.
Usually it's in the form: Bug 123456 - Description of the change. r=reviewer_nickname

So in this case could be
Bug 1133211 - The bookmarks menu dropmarker should not appear below the footer. r=mak

When posting a patch review for review, please use the review? flag instead of the needinfo? flag.

::: browser/components/places/content/menu.xml
@@ +46,5 @@
>            let target = aEvent.target;
>  
>            // Don't draw the drop indicator outside of markers.
>            // The markers are hidden, since otherwise sometimes popups acquire
>            // scrollboxes on OS X, so we can't use them directly.

please also update the comment, as indicated in comment 12

@@ +48,5 @@
>            // Don't draw the drop indicator outside of markers.
>            // The markers are hidden, since otherwise sometimes popups acquire
>            // scrollboxes on OS X, so we can't use them directly.
> +          let betweenMarkers = (this._startMarker.compareDocumentPosition(target) & Node.DOCUMENT_POSITION_FOLLOWING) &&
> +                               (this._endMarker.compareDocumentPosition(target) & Node.DOCUMENT_POSITION_PRECEDING);

Since we try to keep code width as close to 80 chars as possible, you could indent this like

let betweenMarkers =
  (this._start....) &&
  (this._end...);
Attachment #8581723 - Flags: feedback+
Reporter

Updated

4 years ago
Flags: needinfo?(mak77)
Assignee

Comment 25

4 years ago
Posted patch bugfix2.patchSplinter Review
Hope I've done everything right this time, Marco.

Thanks a lot for your help and patience throughout this. Hope to work with you on another bug again some time :)
Attachment #8583183 - Flags: review?(mak77)
Reporter

Updated

4 years ago
Attachment #8581723 - Attachment is obsolete: true
Reporter

Comment 26

4 years ago
Comment on attachment 8583183 [details] [diff] [review]
bugfix2.patch

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

Thank you, it looks good!
Attachment #8583183 - Flags: review?(mak77) → review+
Reporter

Comment 27

4 years ago
If you have some C knowledge, I'd appreciate help with bug 1067054. It's a little bit more complex than this, but not too hard if you know C.
Or bug 901952 if you still look for some easy JS bug.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/718d35643401
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/718d35643401
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.