Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"About:..." bookmarks are protected and cannot be moved + Demand for integration of tabs and bookmarks (e.g. BSP2 with TST) #140

Closed
kenreeser opened this issue Apr 25, 2020 · 27 comments
Closed

"About:..." bookmarks are protected and cannot be moved + Demand for integration of tabs and bookmarks (e.g. BSP2 with TST) #140

kenreeser opened this issue Apr 25, 2020 · 27 comments
Assignees
@aaFn
Labels
enhancement New feature or request external dependency Requires a feature outside of BSP2 to be made available

Comments

@kenreeser
Copy link

I like Bookmark search plus 2 so much that I want to use it in lieu of Firefox's built-in bookmark manager (inside sidebar). However, I discovered an issue in which I'm unable to reorganize bookmarks and folders via drag-&-drop when they're more than 2 folder levels deep in the hierarchy.
@aaFn
Copy link
Owner

aaFn commented Apr 25, 2020 •
edited
Loading

Hello @kenreeser, could you illustrate with a specific scenario of what you re doing which is not working, because I am not able to reproduce here ?

Also, could you first, before reproducing, do a "Reload all bookmarks from FF now" on the options page, to make sure BSP2 is well synchronized with FF ? There are some cases where FF says it has done something, but in fact it didn't do it, and so this generates a shift between the two which can cause that kind of issue later.

@kenreeser
Copy link
Author

kenreeser commented Apr 25, 2020 via email

@aaFn
Copy link
Owner

aaFn commented Apr 26, 2020

Hello @kenreeser, well I tried on my side with L5, L6 and L7 folders, no problem.
It must be something else.

It may be a bug, or simply that for some reason BSP2 considers your folder and bookmarks within it as protected.

1) Protected hypothesis

  • How is the drag mouse cursor behaving when you drag to the place you want to drop on and it doesn't work ?
  • Does it change to a "no-drop" icon ? (see "no-drop" in https://developer.mozilla.org/en-US/docs/Web/CSS/cursor)
  • Or does it remain like the "copy" one (less the plus sign in the little rectangle) ?

2) Bug hypothesis
Could I ask you a trace when you attempt such a drag & drop ?:

  • Open the browser console (Ctrl+Shift+J)
  • Click on the "Console Settings" wheel (top right)
  • Activate "Show Content Messages"
  • Empty it (tray icon on top left)

Then do a failing drag & drop.
Then select all messages you see inside the browser console, and copy/paste them in your response here.
Hope we can catch it ...

3° Something else ?
For a start, could you select the folder where you have a problem, and drag & drop it here, in the GitHub text composition window of your response ?
It will be incomplete as I will miss the names of bookmarks, but at least I will be able to see its structure and URLs of components, and try to reproduce on my side..

On integrating with TST, problem is that for the moment it has too strong "Known restrictions" for BSP2 to work, cf. https://github.com/piroor/treestyletab/wiki/SubPanel-API.
The drag & drop functions with external objects would stop working (I am not sure why TST has that limitation, I managed to get around most of it on my side), and most of all, the fact that we cannot open native context menus .. BSP2 now heavily relies on them ... :-(

@kenreeser
Copy link
Author

kenreeser commented Apr 26, 2020 via email

@kenreeser
Copy link
Author

kenreeser commented Apr 26, 2020 via email

@aaFn
Copy link
Owner

aaFn commented Apr 27, 2020

Hello @kenreeser , ah yes, I had coded initially a principle that any bookmark with "about:.." URL should be protected ..
Probably time to review that rule now. Let me check.

@aaFn
Copy link
Owner

aaFn commented Apr 27, 2020

Ok, I see no more reason to keep them protected, so I will remove that protection in 2.0.76.
Thanks, aaFn.

@aaFn aaFn self-assigned this Apr 27, 2020
@aaFn aaFn added the enhancement New feature or request label Apr 27, 2020
@kenreeser
Copy link
Author

kenreeser commented Apr 27, 2020 via email

@aaFn
Copy link
Owner

aaFn commented Apr 27, 2020 •
edited
Loading

Ah, I had indeed initially the intent to add tabs searching to BSP2, since search was the primary motivation for this add-on. That was including then a specific tree / split subpanel to list all tabs, and use the search box to locate the tab we wanted, as well as manipulate them.

I had this todo in my list as of the first version (2.01):

  • TODO: Add section to show the current tab as a "simili bookmark", allowing:
    • local (inside sidebar, more practical) drag & drop for bookmark creation,
    • on double click or right click menu, expands as a tabs panel for switching very quickly when we have large number of open tabs (similar to "Tab Sidebar" and "Vertical Tabs Reloaded" add-ons),
    • double click back or right click menu on it to restore the Bookmarks view,
    • and most of all to search on them (not available in the two mentioned add-ons).

However, FF made an intelligent move as of version 64, they added a native Tab search feature, accessible through the down arrow at right of all the tabs (appearing when there are more tabs open than can fit on the tabs bar), or by using "%" followed by search terms in the address bar (see https://www.techrepublic.com/article/how-to-search-open-tabs-in-firefox/).

From then I removed the todo, all the more that the remaining value for doing something like that was already covered by TST, and the intent is not to compete with it.

At this stage, I guess a good way to get the two things together would be to find a way with @piroor to collaborate, or to understand and remove at least restrictions #3 and #1 from https://github.com/piroor/treestyletab/wiki/SubPanel-API .. We can try to see if he would be receptive ..

.
On a donation link, I guess I'll see that one day, for now I didn't make my mind on which method to use. If anybody has an advice on good donation systems out there, I'll gladly receive the wiseness of others :-)

Thanks, aaFn.

@kenreeser
Copy link
Author

kenreeser commented Apr 27, 2020 via email

@piroor
Copy link

piroor commented Apr 28, 2020 •
edited
Loading

The restrictions around subpanels in TST looks to be from security system of Firefox's iframe implementation, but I'm positive to improve user experience of subpanels if it can be done without any security concern.

Inspired from this issue, I've experimentally implement more APIs to transfer drag data between TST and a subpanel:

With these changes drag data (URL and title of dragged tabs) can be stolen by helper addons, but I think it is not a security threat because drag data is already public to other applications via OS's API.

The context menu problem is little difficult. To improve user experience around context menu, I think TST must provide APIs to replace all context menu items on subpanels. I think that improving the existing fake context menu API can become a solution.

@aaFn
Copy link
Owner

aaFn commented Apr 28, 2020 •
edited
Loading

Hello @piroor, thank you for your quick answer here.

On context menu:
I had, and still have for FF < 64, for those users which want to stick to earlier versions like FF56, my own fake menus,. However, this is a pain to manage and I switched over to the native FF one very quickly (using by the way your excellent articles on it), and I do not really maintain their content anymore.

My view is that we should find a way to keep using native FF context menus on add-ons running in TST subpanels, so that there are no two codebases for them to manage, corresponding to two environments of run. That should be as transparent as possible, on FF behavior/means used, their oddities/bugs we have to manage (we shouldn't introduce more or others), and on coding logics.

  • What is the problem with menus.overrideContext() in iframes ? (I must confess I didn't experiment)
  • Would it be possible to "proxy" it by TST, in relation with the TST registration API you designed ?

On drag & drop
I see your commits above are mostly about exchanging with TST, correct ?
Beyond the problem of exchanging with the parent TST window, what is the exact problem of drag & drop not working on a subpanel ? Typically, BSP2 is supporting the reception of

// When the dragged element is one of our bookmarks its dt.types will be
//   dt.types        : application/x-bookmark,text/x-moz-place<-xxx>, [text/uri-list,] text/plain, text/html
// When it is a native bookmark, it will be
//   dt.types        : text/x-moz-place<-xxx>
// When it is a native tab, it will be
//   dt.types        : text/x-moz-text-internal
// When it is a link in an HTML page, or in the address bar:
//   dt.types        : text/x-moz-url,text/x-moz-url-data,text/x-moz-url-desc,text/uri-list,text/_moz_htmlcontext,text/_moz_htmlinfo,text/html,text/plain

and is sending

// For a bookmark:	application/x-bookmark and text/x-moz-place,text/x-moz-url,text/plain,text/html
// For a folder:	application/x-bookmark and text/x-moz-place-container,text/x-moz-url,text/plain,text/html
// For a separator:	application/x-bookmark and text/x-moz-place-separator,text/plain,text/html

// application/x-bookmark is for BSP2 internal drag & drops.
// Rest is:// text/x-moz-place<-xxx>:
// Native Bookmark data like  {"title":"Nouveau dossier","id":2238,"itemGuid":"VQ8ulNGyfXk0","instanceId":"jvwImUhXA2rV","parent":2099,"parentGuid":"CLSASmpIpBmQ",
// 							 "dateAdded":1536393478662000,"lastModified":1536393478662000,"type":"text/x-moz-place-container"}
//							{"title":"_  New bookmark","id":2350,"itemGuid":"JE2j0D-5tnpT","instanceId":"jvwImUhXA2rV","parent":2028,"parentGuid":"XmOZ-HAGbfEM",
//							 "dateAdded":1550944109935000,"lastModified":1551218607521000,"type":"text/x-moz-place","uri":"about:blank"}
//							{"title":"","id":2239,"itemGuid":"gORenOTsYJdk","instanceId":"jvwImUhXA2rV","parent":2028,"parentGuid":"XmOZ-HAGbfEM",
//							 "dateAdded":1536396421579000,"lastModified":1551022707690000,"type":"text/x-moz-place-separator"}
// text/plain:
// Native Bookmark data like  New folder3\nabout:blank\n--------------------\nabout:blank
//							about:blank
//							--------------------
// text/html:
// Native Bookmark data like  <DL><DT>New folder3</DT><DD><A HREF="about:blank">New bookmark5</A></DD><DD><HR></DD><DD><A HREF="about:blank">New bookmark4</A></DD></DL>
//							<A HREF="about:blank">_  New bookmark</A>
//							<HR>
  • Any problem, apart from that bug of course https://bugzilla.mozilla.org/show_bug.cgi?id=1531539 :-), in getting direct drag & drop events on the subpanel, from or to external areas, in FF (that can be another instance of BSP2 inside a tab, or in another FF window), or outside FF (e.g. Thunderbird, notepad ..) ?
  • Wouldn't it be possible to agree on particular dt.types and formats for D&D to allow exchange between the subpanel and the TST parent ?

@aaFn aaFn changed the title Unable To Reorganize Bookmarks & Folders When More Than 2 Folders Deep "About:..." bookmarks are protected and cannot be moved + Demand for integration of tabs and bookmarks (e.g. BSP2 with TST) Apr 28, 2020
@aaFn
Copy link
Owner

aaFn commented Apr 28, 2020

As an aside, but that was the initial topic of that issue 😀, 2.0.76 is out now, and removed the protection on "about:..." bookmarks. I changed the title to reflect both issues.

@kenreeser
Copy link
Author

kenreeser commented Apr 29, 2020 via email

@piroor
Copy link

piroor commented Apr 29, 2020

@aaFn I've updated experimental APIs and documented them:

The document contains descriptions about technical background why those approach are required. I hope that they become answers for your questions.

And those changes are already merged to the master of TST. Now you can try development builds of TST and a cloned bookmarks panel for the subpanel area.

I think that supporting of TST's subpanel based on this way is too bothered work, so I cannot recommend that to you, sorry... (As described at the top of the API document, initially I created the API to demonstrate impracticableness of the subpanel concept. So they are definitely hard to use on actual usecase.)

@aaFn
Copy link
Owner

aaFn commented Apr 29, 2020 •
edited
Loading

Hello @piroor , good engineering job, I am impressed :-)

Agree, as I was reading your links, I was getting a growing feeling of impracticability. Doable, but hard to keep a common code base able to run both inside a subpanel of TST and on its own also.
And full of little peculiarities which will differ between "inside the frame" and "on its own", as things are not so clean everywhere in FF. I spent much time to get around things not working exactly as expected nor documented like this, or like missing dragleave/dragexit events in some specific cases, and I feel running as a subpanel inside another add-on is going to bring its whole specific lot.

All of that complexity / impracticability seems to be rooted to the fact that opening another add-on code in an iframe of TST sidebar is apparented to a content script. I was not aware of that, and I must say I am a bit surprised, but I am much younger than you at experimenting with FF and WE, and thinking twice it may look logical ..

I believe I need to try a few things to finish convincing myself, and when it's the case, maybe add my voice to https://bugzilla.mozilla.org/show_bug.cgi?id=1328776

Meanwhile, thanks for all that deep thinking 🖖

@kenreeser
Copy link
Author

kenreeser commented May 5, 2020 via email

@aaFn
Copy link
Owner

aaFn commented May 5, 2020

Hello @kenreeser , well I tried on my side and no such thing is happening.
When you compare the content of that fodler with the native bookmark sidebar view, do they look the same ? Could you copy me here the content of that folder ?

@kenreeser
Copy link
Author

kenreeser commented May 6, 2020 via email

@aaFn
Copy link
Owner

aaFn commented May 10, 2020

Hello @kenreeser , any news ? Still trying to reproduce on my side but with no luck.
Thanks, aaFn.

@kenreeser
Copy link
Author

kenreeser commented May 11, 2020 via email

@aaFn
Copy link
Owner

aaFn commented May 12, 2020

Hello @kenreeser, ok, let's watch out and see if / when it re-happens.

On the sharing of sidebar by TST and BSP2:

  • Our joint conclusion with piro is that BSP2 inside TST is not really workable, so I added my voice and my vote to https://bugzilla.mozilla.org/show_bug.cgi?id=1328776#c41
  • The reverse is also true, if BSP2 was creating a subpanel, and try to run TST inside it, that would currently make TST break as I see it, because subpanel doesn't provide all the features TST needs. Then BSP2 and TST would have to collaborate in reverse way for BSP2 to provide an API, and TST to use it ... same conclusion as above at this stage. The solution is really what is in bugzilla.
  • Then that would only leave for now the option that I add TST like features to BSP2, but I do not want to duplicate the very good work of piro.

Let me know if you see another option ? Else, I am afraid I don't see a way.

@aaFn
Copy link
Owner

aaFn commented Jun 14, 2020 •
edited
Loading

No news = good news I presume :-)
Please open a new issue if the problem occurs again, and let's keep that one for tracking integration with TST, as external dependency on firefox to enable the feature. Thanks aaFn.

@aaFn aaFn closed this as completed Jun 14, 2020
@aaFn aaFn added the external dependency Requires a feature outside of BSP2 to be made available label Jun 14, 2020
@kenreeser
Copy link
Author

kenreeser commented Jun 25, 2020 via email

@aaFn
Copy link
Owner

aaFn commented Jun 29, 2020

Hello @kenreeser , I guess we should open a new issue to properly track the demand, however here I am not sure to understand the demand yet.

Currently, in the Bookmark property window, both Name: and Address: fields grow or decrease with the property dialog, so for me it does what you want (I can verify onlyl on Windows and Linux though, not on IOS as I do not have access to any Apple device).

The only catch is that they have a minimum size (the default one of input fields), so when the window is too small, their end is not visible as it goes beyond the right window limit, and there is no scrollbar appearing to be able to see their right end.
However that does not seem to be what you are referring to.

Could you clarify a bit more ?
Thanks, aaFn

@kenreeser
Copy link
Author

kenreeser commented Jun 29, 2020 via email

@aaFn
Copy link
Owner

aaFn commented Jun 29, 2020

Ok, no problem, I prefer that, I was getting puzzled 😀 .
Thanks, aaFn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aaFn aaFn

Labels
enhancement New feature or request external dependency Requires a feature outside of BSP2 to be made available
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@piroor @kenreeser @aaFn