460678 - make it possible to scroll content-area XUL widgets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sergey Zh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ru; rv:1.9.0.3) Gecko/2008091700 SUSE/3.0.3-1.1 Firefox/3.0.3
Build Identifier: 

On XUL webpage in <scrollbox>  scrollbar is not shown and it is impossible to scroll.
As an example link to simple scrollbox at this page: https://bugzilla.mozilla.org/attachment.cgi?id=169657

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

this is seen in the mochitests:
dom/tests/mochitest/general/test_offsets.xul

this causes the script to error out and 210 test cases do not run and 3 fail.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

We support panning now in chrome-level scrollboxes. We should look into supporting that for content too.

Comment 3

[Ben Combee (:bcombee)]

This sounds like a case similar to iframe scrolling.

Comment 4

[Ben Combee (:bcombee)]

Here's a common test case.  Go to about:config.  You can pan to the bottom of the page, but you can't scroll through the list and see what's beyond that page.  You also can't double click on an entry to edit it, since double click is treated as a zoom action, but that's a different bug.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

The first part in comment #4 is bug 491072.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> The first part in comment #4 is bug 491072.

Not really. He is saying that panning works, but scrolling the listbox of config options does not.

Comment 7

[Adric Net]

The test URL attached above loads a blank grey screen on Fennec 1.0b2 on Mac OS X 10.5.7 and this page http://www.listbox.com/member/archive/247/2009/08/sort/time_rev/page/1/entry/1:48/20090809121534:DA6C85D8-84FF-11DE-A331-D2E511F6C9C8/
loads but is not usable due to the missing scrollbar (iframe). Meanwhile, about:config only scrolls down to about the line browser.anchor.color.

That listbox.com page (and the attached example above) loads and behaves correctly on Firefox 3.5.2 (same Mac OS X 10.5.7 system). I don't see a newer Fennec image for Mac to check against on ftp, sorry.

Comment 8

[Uwe Kaminski]

An other example is the configuration of the shareaholic extension in the extension area. Without the possibility of scrolling the configuration this plugin is useless. In about:config the problem is smaler because it's possible to search for a string what makes the list shorter and shorter.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dirty slooww patch to scroll about:config

it's just an experiment to see how hard is it to scroll about:config. The patch can only scroll xul:tree vertically, and it is sloooooooooooow cause of the scrollByLines tree method.

Comment 11

[Ben Combee (:bcombee)]

Comment on attachment 395304 [details] [diff] [review]
dirty slooww patch to scroll about:config

1) In panTree, use Math.round instead of Math.floor so you get correct behavior with this._currentY is negative.

2) you need to handle detecting that the tree moved and act accordingly.  You can get rid of "panned" and just return from the test expression.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Should we consider making a special customDragger for this?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ben, I don't want to take the bug, I have just tried an experiment cause I wanted to scroll about:config but I'm not sure that's the right method. It's really slow :(

Comment 14

[Ben Combee (:bcombee)]

Ultimately, I think the issue is how do we handle XUL elements with the tile cache.  The about:config page kinda works like an iframe with that tree element inside, which means every move invalidates all those tiles and redraws the world.  We'd really rather have this be it's own panel and run outside of tile cache or have it expand to maximum height from the start.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

vote for not blocking1.0

Comment 18

[Fabrice Desré]

Attachment: new patch for XUL content

This patch provides support for panning elements in XUL documents loaded in the content :
- uses scrollBoxObject or boxObject.QI(nsIScrollBoxObject) when available
- special case for trees, for which we generate a custom scrollBoxObject

Can be tested with about:config

Comment 19

[Fabrice Desré]

Attachment: xul test file.

Comment 20

[Fabrice Desré]

Attachment: updated patch

Cleaned up unused code and added support for scrollable div elements. For instance, go to http://www.quirksmode.org/dom/tests/elementdimensions.html and play with the "switch content" link.

The html part should probably go to bug 511432

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 410089 [details] [diff] [review]
updated patch

>diff -r 5cc6cea740ac chrome/content/InputHandler.js

I'm wondering if we couldn't move these creator functions out to global functions?

>   /**
>+   * builds a minimal implementatino of scrollBoxObject for trees
>+   */
>+  createTreeScrollBox: function createTreeScrollBox(tree) {
>+    let treeBox = tree.boxObject.QueryInterface(Components.interfaces.nsITreeBoxObject);

Use Ci

>+    let sbo = {

>+      scrollBy: function(dx, dy) {

>+        let targetRow = Math.floor(this.targetY / treeBox.rowHeight);
>+        if ((targetRow + treeBox.getPageLength()) > treeBox.view.rowCount) {
>+          targetRow = treeBox.view.rowCount - treeBox.getPageLength();
>+          this.targetY = targetRow * treeBox.rowHeight;

Put treeBox.rowHeight, treeBox.getPageLength() and treeBox.view.rowCount into local variables. I'm just worried about xpconnect performance in the scrollBy method.

>+  createDivScrollBox: function createDivScrollBox(div) {
>+    let sbo = {
>+      

Remove the blank line

>-  getScrollboxFromElement: function getScrollboxFromElement(elem) {

>+    let elem = event.target;
>+    
>+    

Remove the extra blank line

>+    // Check if we are in a scrollable html element
>+    if (target && target.namespaceURI == "http://www.w3.org/1999/xhtml") {

Would if (target && target instanceof HTMLElement) { work here?

>+      let win = Browser.selectedBrowser.contentWindow; 

Could we get win from the target? let win = target.ownerDocument.defaultView;

>+    // Check if we are in XUL land
>+    if (target && target.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul") {

Would if (target && target instanceof XULElement) { work here?

>+      for (; target; target = target.parentNode) {
>+        if (target.localName == "tree") {

I'm looking for an instanceof version of this test, but haven't found it yet :(

Nice patch!

Comment 22

[Fabrice Desré]

Attachment: new patch

Corrections made according to comments.

For the tree element, would this be better to test if we can QI to nsIDOMXULTreeElement ?

Comment 23

[Benjamin Stover (:stechz)]

For me, the latest patch doesn't pan tree elements (I tried testcase and about:config).

For content, this really needs to go through browser.js's dragMove, so that once something can't pan anymore, the next scrollable parent is dragged.

Comment 24

[Fabrice Desré]

Attachment: tree scrolling fix

This patch scrolls trees as expected, and makes sure that the scrollable part does not include the tree header row.

Benjamin, I don't think this should go to browser.js dragMove. For instance, when you pan a scrolling div, you don't want the full page to move once you're at the panning limits of the div.

Comment 25

[Benjamin Stover (:stechz)]

> Benjamin, I don't think this should go to browser.js dragMove. For instance,
> when you pan a scrolling div, you don't want the full page to move once you're
> at the panning limits of the div.

Hmm.  Why would we treat iframes different than overflow:auto elements or tree elements?  The reason we do this for iframes is so that if the user's entire viewport is an iframe, there is still a way to get to the browser controls.  I think the same argument applies here.

Let me know if I'm missing something.

Comment 26

[Fabrice Desré]

This is a good point. Ideally I'd like to have two different behaviors 
- iframes-like if the scrolling part is the entire viewport
- the one implemented here in other cases

does it makes sense ?

Comment 27

[Fabrice Desré]

Attachment: new patch

New patch implementing the behavior described in comment 25.

Testable with :
about:config
http://www.quirksmode.org/dom/tests/elementdimensions.html
https://bug460678.bugzilla.mozilla.org/attachment.cgi?id=410052

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 414495 [details] [diff] [review]
new patch

Works very well on the test cases

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/e642dd4ef4d1

Comment 30

[Aakash Desai [:aakashd]]

It works, but the implementation is pretty fug on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091201 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091201 Firefox/3.7a1pre Fennec/1.0b5

follow-up bug filed: bug 523137