element.clientHeight & element.scrollHeight incomplete implement #19280

tigercosmos · 2017-11-18T18:29:27Z

With the following test.html:

    <body>
        <div id="scroller" style="height: 100px; width: 100px; overflow: scroll; background: red;">
            <div style="background: green; margin-top: 100px; width: 100px; height: 100px;"></div>
        </div>
  
        <script>
            var scroller = document.getElementById("scroller");
            console.log(scroller.clientHeight)
            console.log(scroller.scrollHeight) 
        </script>
    </body>

Firefox will get:

96  <--- this is caused by scrollbar
200

Servo will get:

100 <---- servo count scrollbar currently
100 <---- this is wrong

As clientHeight is a known issue, we may need to check out scrollHeight

This might be an issue related to #19268

servo/components/script/dom/element.rs

Lines 373 to 376 in b2b51d3

    
           fn has_overflow(&self) -> bool { 
        
               self.ScrollHeight() > self.ClientHeight() || 
        
               self.ScrollWidth() > self.ClientWidth() 
        
           }

tigercosmos · 2017-11-19T18:05:19Z

I figure out that ScrollHeight cannot deal with margin-top: 100px;
If we change the second line of this:

        <div id="scroller" style="height: 100px; width: 100px; overflow: scroll; background: red;">
            <div style="background: green; margin-top: 100px; width: 100px; height: 100px;"></div>
        </div>

to
~~margin-top: 100px~~ and height: 190px;
ScrollHeight will get correct answser 190

tigercosmos · 2017-11-20T05:58:27Z

Seems scrollHeight will return max(margin-top, height)
As shown in following:

scroller.scrollHeight will get 100

<style>
    #scroller {
        height: 100px;
        width: 100px;
        overflow: hidden;
        background: red;
    }

    #container {
        background: green;
        margin-top: 100px;
        width: 100px;
        height: 30px;
    }
</style>
<div id="scroller">
        <div id="container"></div>
 </div>

scroller.scrollHeight will get 190

<style>
    #scroller {
        height: 100px;
        width: 100px;
        overflow: hidden;
        background: red;
    }

    #container {
        background: green;
        margin-top: 100px;
        width: 100px;
        height: 190px;
    }
</style>
<div id="scroller">
        <div id="container"></div>
 </div>

I also test changing margin to padding, and padding will run correctly.

tigercosmos · 2017-11-26T12:04:06Z

Something might be related:

servo/components/layout/query.rs

Lines 135 to 139 in fe4139b

    
           fn node_scroll_area(&self) -> NodeGeometryResponse { 
        
               NodeGeometryResponse { 
        
                   client_rect: self.0.lock().unwrap().scroll_area_response 
        
               } 
        
           }

servo/components/layout/query.rs

Lines 628 to 656 in fe4139b

    
           pub fn process_node_scroll_area_request< N: LayoutNode>(requested_node: N, layout_root: &mut Flow) 
        
                   -> Rect<i32> { 
        
               let mut iterator = UnioningFragmentScrollAreaIterator::new(requested_node.opaque()); 
        
               sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator); 
        
               match iterator.overflow_direction { 
        
                   OverflowDirection::RightAndDown => { 
        
                       let right = max(iterator.union_rect.size.width, iterator.origin_rect.size.width); 
        
                       let bottom = max(iterator.union_rect.size.height, iterator.origin_rect.size.height); 
        
                       Rect::new(iterator.origin_rect.origin, Size2D::new(right, bottom)) 
        
                   }, 
        
                   OverflowDirection::LeftAndDown => { 
        
                       let bottom = max(iterator.union_rect.size.height, iterator.origin_rect.size.height); 
        
                       let left = max(iterator.union_rect.origin.x, iterator.origin_rect.origin.x); 
        
                       Rect::new(Point2D::new(left, iterator.origin_rect.origin.y), 
        
                                 Size2D::new(iterator.origin_rect.size.width, bottom)) 
        
                   }, 
        
                   OverflowDirection::LeftAndUp => { 
        
                       let top = min(iterator.union_rect.origin.y, iterator.origin_rect.origin.y); 
        
                       let left = min(iterator.union_rect.origin.x, iterator.origin_rect.origin.x); 
        
                       Rect::new(Point2D::new(left, top), iterator.origin_rect.size) 
        
                   }, 
        
                   OverflowDirection::RightAndUp => { 
        
                       let top = min(iterator.union_rect.origin.y, iterator.origin_rect.origin.y); 
        
                       let right = max(iterator.union_rect.size.width, iterator.origin_rect.size.width); 
        
                       Rect::new(Point2D::new(iterator.origin_rect.origin.x, top), 
        
                                 Size2D::new(right, iterator.origin_rect.size.height)) 
        
                   } 
        
               } 
        
           }

tigercosmos · 2017-11-30T16:58:04Z

servo/components/layout/query.rs

Lines 874 to 886 in fe4139b

    
           pub fn process_margin_style_query<N: LayoutNode>(requested_node: N) 
        
                   -> MarginStyleResponse { 
        
               let layout_node = requested_node.to_threadsafe(); 
        
               let style = &*layout_node.as_element().unwrap().resolved_style(); 
        
               let margin = style.get_margin(); 
        
               MarginStyleResponse { 
        
                   top: margin.margin_top, 
        
                   right: margin.margin_right, 
        
                   bottom: margin.margin_bottom, 
        
                   left: margin.margin_left, 
        
               } 
        
           }


        Auto merge of #19469 - tigercosmos:z1, r=<try>

[WIP] fix scroll area about scrollHeight and scrollWidth  Currently `scrollHeight` will get the result that `max(height + padding, margin)` It should be `scrollHeight = height + padding + margin` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19280 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19469)


        Auto merge of #19469 - tigercosmos:z1, r=<try>

[WIP] fix scroll area about scrollHeight and scrollWidth  Currently `scrollHeight` will get the result that `max(height + padding, margin)` It should be `scrollHeight = height + padding + margin` Same as `scrollWidth` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19280 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19469)

tigercosmos mentioned this issue Nov 18, 2017

simple_scroll_to_fragment.html is now a perma-failure #19268

Open

This was referenced Dec 2, 2017

margin will not be hidden when overflow is hidden or scroll #19466

Open

[WIP] fix scroll area about scrollHeight and scrollWidth #19469

Closed

servo / servo

element.clientHeight & element.scrollHeight incomplete implement #19280

element.clientHeight & element.scrollHeight incomplete implement #19280

tigercosmos commented Nov 18, 2017 •

edited

tigercosmos commented Nov 19, 2017

tigercosmos commented Nov 20, 2017 •

edited

tigercosmos commented Nov 26, 2017

tigercosmos commented Nov 30, 2017

servo / servo

Join GitHub today

element.clientHeight & element.scrollHeight incomplete implement #19280

element.clientHeight & element.scrollHeight incomplete implement #19280

Comments

tigercosmos commented Nov 18, 2017 • edited

tigercosmos commented Nov 19, 2017

tigercosmos commented Nov 20, 2017 • edited

tigercosmos commented Nov 26, 2017

tigercosmos commented Nov 30, 2017

tigercosmos commented Nov 18, 2017 •

edited

tigercosmos commented Nov 20, 2017 •

edited