very bad font inflation when filing a new bug

RESOLVED WONTFIX

Status

()

Core
Layout
P1
normal
RESOLVED WONTFIX
6 years ago
11 months ago

People

(Reporter: Ehsan, Assigned: dbaron)

Tracking

(Depends on: 1 bug, {productwanted, uiwanted})

unspecified
Other
Other
productwanted, uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 -, fennec+)

Details

(Whiteboard: [readability])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
try filing a new bug. some of the description messages get inflatrd, but most of them are not. also the text on the search button is cropped to "Se".
(Reporter)

Comment 1

6 years ago
generally, font inflation is broken on bugzilla.

Updated

6 years ago
Blocks: 627842
(Assignee)

Comment 2

6 years ago
Did this get better or worse when bug 706193 landed?
(Assignee)

Comment 3

6 years ago
Or, alternately, if you're using a build within the past few days, does it get better or worse if you set the font.size.inflation.lineThreshold preference to 0?
(Reporter)

Comment 4

6 years ago
(In reply to David Baron [:dbaron] from comment #3)
> Or, alternately, if you're using a build within the past few days, does it
> get better or worse if you set the font.size.inflation.lineThreshold
> preference to 0?

That is better.  Now most of the text on the page is consistently inflated, except for links to the products.  The text on the search button is still cropped.
(Reporter)

Updated

6 years ago
blocking-fennec1.0: --- → ?

Updated

6 years ago
Duplicate of this bug: 748096
blocking-fennec1.0: ? → +
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Whiteboard: [readability]

Updated

6 years ago
Duplicate of this bug: 748407

Updated

6 years ago
Assignee: nobody → sjohnson

Updated

6 years ago
Duplicate of this bug: 748054
See:
bug 748054, comment 3
bug 748054, comment 4

for additional info on this issue.
From bug 748054, commnent 3:
> Fixing ycombinator (bug 707195) would require the patch series I've already written in 
> that bug, possibly plus patches to make overflow!=visible (i.e., nsHTMLScrollFrame) 
> not be an inflation flow root (which is what should fix reddit), 

This makes sense. I can start working on this.

> plus patches to make *some* table cells not be inflation flow
> roots (but leave others, otherwise we'd  regress nytimes.com).

This doesn't make sense to me... nytimes doesn't have tables in the layout that I can see? Are you saying that we'd regress the nytimes footer bug _in general_ if we make a change where all table cells were not flow roots?

> I think the underlying heuristic could perhaps be something 
> like whether there's inflatable text in more than one column of the table, but 
> figuring out the right way to do that and how to do it efficiently is likely to be 
> tricky.

So, this would work for Damon's case, but it's not going to work for the bugzilla case - bugzilla has (for the most part) a single table, with what appears to be at most 4 columns.

Updated

6 years ago
Priority: -- → P1
What if we did something like this:

I propose adding a new nsIFrame pointer into nsTableFrame, called mLastFontContainerCell, which will only be used during reflow. (So we might be able to put it in the temporary reflow state that gets passed around). Then, what I would like to do is "bubble up" the font inflation container status from table cells to nsTableFrames IF a table frame contains two or more columns that are themselves font inflation containers. (Thus, two cells that would contain enough text to trigger font inflation by themselves).

Basically, if we have multiple tables nested within each other, on successive reflows, the tables would bubble up the font inflation status so that eventually, if there is enough text spread across all of the frames, the uppermost table would be the font inflation container/flow root.

During reflow of nsTableCellFrame:
----------------------------------

If (this frame is a font inflation container) {
  parentTable <- the parent table frame of this frame
  if (parentTable is a font inflation container) {
    remove the font inflation container and font inflation flow root
    state bits from this frame
  } else if (parentTable.mLastFontContainerCell != nsnull and
             column index of parentTable.mLastFontContainerCell != column index of this)        {
               remove the font inflation container and font inflation flow root 
               state bits from this frame.

               set font inflation container and flow root bits on parent frame**
             }
    else {
      parentTable.mLastFontContainerCell <- this
    }
}

** During this procedure, we'd probably have to "bubble up" the font inflation container status to the parent frame, if the table is the only child of its parent. Otherwise, the iterative procedure wouldn't work.

PROBLEMS:
---------
There are a couple of issues with this approach. Probably the most obvious is that it could regress situations where we don't want to inflate tables. I tried to get around this by only setting the table to be a flow root if we have one cell in at least two columns, that, by themselves would be inflated. I'm not sure how stable this will be, though.

Another problem is that if a table is nested within multiple divs, all of which are within another table, for example, like:

<table> 
...
<div id="div1">
...
<div id="div2">
<table>
 ...
</table>
...
</div>
</div>
</table>

then we still might run into inconsistent font inflation. This could happen if folks use tables for layout, which is still (I think) pretty common.

I guess I'm just looking at this as a potential starting place for this. Refinement is definitely necessary, and I'm not even sure this will work at all.

Updated

6 years ago
Depends on: 707195
(Assignee)

Comment 11

6 years ago
(In reply to Scott Johnson (:jwir3) from comment #9)
> This doesn't make sense to me... nytimes doesn't have tables in the layout
> that I can see? Are you saying that we'd regress the nytimes footer bug _in
> general_ if we make a change where all table cells were not flow roots?

There's a piece around the middle of the page that's in a table-layout:fixed table inside an overflow:hidden container -- it's the part that contains the horizontally-scrollable "INSIDE NYTIMES.COM" section that divides the two main parts of the page.

Comment 12

6 years ago
Re-nomming this one to force a re-test against the builds posted for bug 707195.
blocking-fennec1.0: + → ?

Updated

6 years ago
Keywords: qawanted
blocking-fennec1.0: ? → +
Keywords: productwanted, uiwanted
Discussed in triage: in the endgame, this is not a blocker
tracking-fennec: --- → 15+
blocking-fennec1.0: + → -
qawanted: assigned to eric to retest bugzilla on latest nightly.

Updated

6 years ago
QA Contact: xwei

Comment 15

6 years ago
Created attachment 640274 [details]
Font Inflation on SeaMonkey

As of today's nightly, the SeaMonkey section still seems to be inflated incorrectly.

Updated

6 years ago
Priority: P1 → --
tracking-fennec: 15+ → +

Updated

5 years ago
Priority: -- → P1
Dropping qawanted based on comment 15.
Keywords: qawanted

Updated

4 years ago
Assignee: jaywir3 → nobody
David, what do you want to do with these old font inflation bugs?
Flags: needinfo?(dbaron)
QA Contact: xwei
assigning to dbaron
Assignee: nobody → dbaron
QA Contact: xwei
(Assignee)

Comment 19

4 years ago
I'm hoping to find a new owner for font inflation code at some point, but don't currently have a candidate.

Alternatively, I might have a chance to look into this at some point, but probably not in the next month or so.  (Heuristics are hard, and the basic problem we need to solve with them here is distinguishing tables whose cells are logically in a single flow of text from tables whose cells are logically separate flows of text, and doing this extremely efficiently in a way that doesn't require lots of updates during incremental layout resulting from addition of content.  Although it might help if we separated the decisions about disabling font inflation because the table is layout-ish from the decisions about what is a separate flow.)
seems fine on latest nightly (55).
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 21

11 months ago
We disabled the feature.
Flags: needinfo?(dbaron)
Resolution: WORKSFORME → WONTFIX
You need to log in before you can comment on or make changes to this bug.