Bug 22333 – Text wrapping algorithm for small screens (eg. Mobile screens)

• Component: Layout and Rendering
• Status: NEW

[Biment Srivastav]

I am attaching patch to avoid too much of horizontal scrolling in small devices, we can improve the algorithm for text wrapping. I have added a new entry in WebCore::Settings to make usage of this algorithm configurable.

[Biment Srivastav]

Created attachment 25233 [details]
Text Wrapping algorithm

[Simon Hausmann]

Comment on attachment 25233 [details]
Text Wrapping algorithm


> +        void setTextWrappingEnabled(bool);
> +        bool textWrappingEnabled() const { return m_textWrappingEnabled; }

Can you elaborate a bit what exactly this feature/setting implements?

I have doubts about the term "TextWrapping" as WebKit certainly wraps text by default. I think this needs a more specific name.

> @@ -1579,6 +1581,54 @@
>      if (resolver.position().atEnd())
>          return resolver.position();
>  
> +    int frame_width;
> +    RenderView* v = view();
> +    Settings *settings = NULL;
> +    if(v && v->frameView()){
> +       frame_width = v->frameView()->visibleWidth();
> +       frame_width = max(frame_width - leftOffset(m_height), 0);
> +       settings = v->frameView()->frame()->page()->settings(); 
> +    }
> +    if((frame_width < width) && settings && settings->textWrappingEnabled()){
> +       // try to figure out if we want to use text width limit  
> +       bool fixedParent = false;
> +       RenderObject* p = this;
> +       // if we have a fixed height parent box, dont use it  
> +       for (int n=4;p && n>0;n--){
> +            if (p->style()->height().isFixed()) {
> +                fixedParent = true;
> +                break;
> +            }
> +            p = p->parent();
> +       }
> +       // check if there is enough text  
> +       if (!fixedParent) {
> +           RenderObject *o = resolver.position().obj;
> +           unsigned charsFound = 0;
> +           unsigned mostCharsInElement = 0;
> +           while (o) {
> +                 if (o->isText()) {
> +                     RenderText *t = static_cast<RenderText *>(o);
> +                     mostCharsInElement = max(mostCharsInElement, t->textLength());
> +                     charsFound += t->textLength();
> +                     if (charsFound>100 && mostCharsInElement>20) break;
> +                 }
> +                 if (o->isWidget()) {
> +                     charsFound = 0;
> +                     break;
> +                 }
> +             o = bidiNext( resolver.position().block, o);
> +           }
> +          // need at least x amount of characters in the block  
> +          // also if longest single text node (not broken by inlines) is very short  
> +          // it is likely some menu thing and we don't apply the limit  
> +          if (charsFound > 30 && mostCharsInElement > 25)
> +              width = frame_width;
> +
> +       }
> +    }

This code has some coding style issues. There are for example spaces missing between the semicolons separating the arguments to the for() statement, misplaced '*' signs for pointers or the use of NULL instead of 0 for declare a null pointer.

[Mark Rowe (bdash)]

It doesn't seem like this is the sort of thing that an application would want to switch at runtime.  That suggests it may be better as a compile-time option similar to USE(LOW_BANDWIDTH_DISPLAY).

[Biment Srivastav]

Created attachment 25264 [details]
Text wrapping algorithm for fixed width

I am attaching a patch with compile-time flag.

Brief description about Text wrapping for fixed width: -
To avoid horizontal scrolling while reading text content in small display, we override the fixed width with screen width for the the block which contains text.

I have also attached screen-shots which illusrates the usage of the feature.

normalTextWrappinh.jpg - result without algorithm
resultWithAlgorithm.jpg - result with algorithm

[Biment Srivastav]

Created attachment 25265 [details]
Text wrapping algorithm for fixed width

I am attaching a patch with compile-time flag.

Brief description about Text wrapping for fixed width: -
To avoid horizontal scrolling while reading text content in small display, we override the fixed width with screen width for the the block which contains text.

I have also attached screen-shots which illusrates the usage of the feature.

normalTextWrappinh.jpg - result without algorithm
resultWithAlgorithm.jpg - result with algorithm

[Mahesh Kulkarni]

Created attachment 25278 [details]
screen shot: normal wrapping

[Mahesh Kulkarni]

Created attachment 25279 [details]
screen shot: resulted wrapping

[Mahesh Kulkarni]

Created attachment 25280 [details]
Patch for review

changes are in bidi.cpp findNextLineBreak. Algorithm wraps text even in case of fixed width for LOW_BANDWIDTH_DISPLAY.

[Darin Adler]

(In reply to comment #8)
> changes are in bidi.cpp findNextLineBreak. Algorithm wraps text even in case of
> fixed width for LOW_BANDWIDTH_DISPLAY.

LOW_BANDWIDTH_DISPLAY seems like a separate switch. I don't think these two should be tied together.

[Antti Koivisto]

Comment on attachment 25280 [details]
Patch for review

I wrote the original version of this code many years ago so I probably should not be one to review it. Anyway

- I know what it does and why and how but it is probably not obvious to others. You should include more comments to the code as well as to the change log explaining it better.
- As Darin said, this has nothing to do with LOW_BANDWIDTH_DISPLAY so it should not be behind that feature flag
- Coding style has many errors http://webkit.org/coding/coding-style.html
- It uses magic constants: 100, 30, 25. These should be at least be replaced with named constants.
- frameWidth may be used uninitialized
- How did you test it? Are you sure this code works as expected with the current bidi.cpp? The surrounding code has seen many changes yet this looks very similar it used be.

All in all I'm not sure this code should checked in. It is pretty evil and I'm not sure if anyone else is going to want to use it. It will break easily since there are no buildbots building it. It might be better to apply this sort of changes internally in organizations that want them. I don't see many benefits to anyone in having it in public tree.